Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add export of Flux-dev transformer and uploading to Azure #717

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sogartar
Copy link
Contributor

@sogartar sogartar commented Dec 19, 2024

There was a slight difference in the Flux schnell and dev variants. Namely, dev has a guidance layer and schenll does not.

Fixed some tensor argument element value types as they were always passed as f32 while some of them should use the model's dtype.

Refactored a bit the Flux transformer export boilerplate.

Added a script that uploads models to Azure. Right now it uploads the Flux transformer models.
This can become a part of the CI jobs at some point.

There was a slight difference in the Flux schnell and dev variants.
Namely, dev has a guidance layer and schenll does not.

Fixed some tensor argument element value types as they were always
passed as f32 while some of the should use the models dtype.

Refactored a bit the Flux transformer export boilerplate.

Added a script that uploads models to Azure. Right now it uploads the
Flux transformer models.
This can become a part of the CI jobs at some point.
@sogartar sogartar requested a review from IanNod December 19, 2024 22:51
Comment on lines 16 to 17
azure-identity>=1.19
azure-storage-blob>=12.24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put these new requirements in a dev requirements file or an "extras" so they don't spill into the packages we publish unconditionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sharktank package currently has two requirements files, one for the standard install and one for the sharktank[testing] extra:

[tool.setuptools.dynamic.dependencies]
file = ["requirements.txt"]
[tool.setuptools.dynamic.optional-dependencies]
testing = {file = ["requirements-tests.txt"]}

Maybe testing is a good fit for this? We can also add a new category as needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not required by any tests right now. I added a dev package component. And put them in another file requirements file.

@@ -0,0 +1,51 @@
from ..utils.azure import upload_all_models
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright headers at the tops of files please

Suggested change
from ..utils.azure import upload_all_models
# Copyright 2024 Advanced Micro Devices, Inc.
#
# Licensed under the Apache License v2.0 with LLVM Exceptions.
# See https://llvm.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
from ..utils.azure import upload_all_models

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -12,3 +12,6 @@ datasets
# Serving deps.
fastapi>=0.112.2
uvicorn>=0.30.6

azure-identity>=1.19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add azure as a requirement here. It is very nice for us developers, but our users will likely not have a use for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw @ScottTodd's comment on this above. I agree that maybe we should add another dev or extras requirements file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I answered on the other thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants