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 bisect_model API under ct.models.utils #2286

Merged
merged 11 commits into from
Jul 29, 2024
Merged

Conversation

jakesabathia2
Copy link
Collaborator

@jakesabathia2 jakesabathia2 commented Jul 24, 2024

  • This is our proposed solution of issue unet Chunking not supported in coremltools>7.1 and <7.1 does not support dynamic reshape #2264
  • Basically, we moved / and did some slight modification of the code in ml-stable-diffusion repo, and put the utils of bisecting a mlpackage under scope of ct.models.utils.bisect_model
  • After this fix is in, I am going to put another PR in ml-stable-diffusion, and make the chunk_mlprogram.py directly calling into this new API, so that any further implementation details changes in coremltools will not break the script itself.

Testing:
https://gitlab.com/coremltools1/coremltools/-/pipelines/1389709376
https://gitlab.com/coremltools1/coremltools/-/pipelines/1389735502
https://gitlab.com/coremltools1/coremltools/-/pipelines/1392495182

@1duo
Copy link
Collaborator

1duo commented Jul 24, 2024

Nice! Would be great to include example usages / tests.

@jakesabathia2 jakesabathia2 force-pushed the eng/PR-bisect-model branch 11 times, most recently from fe4785d to c1a24ff Compare July 25, 2024 18:02
@jakesabathia2 jakesabathia2 changed the title [WIP] Add bisect_model API under ct.models.utils Add bisect_model API under ct.models.utils Jul 25, 2024
@jakesabathia2
Copy link
Collaborator Author

Nice! Would be great to include example usages / tests.

Thanks! Just added.

@aseemw
Copy link
Collaborator

aseemw commented Jul 25, 2024

Can you also add a small section describing the util in the docs, along with its use case (when to use it, e.g. SD model thats big) : https://apple.github.io/coremltools/docs-guides/source/mlmodel-utilities.html

1duo
1duo previously approved these changes Jul 25, 2024
coremltools/models/utils.py Outdated Show resolved Hide resolved
TobyRoseman
TobyRoseman previously approved these changes Jul 25, 2024
coremltools/models/utils.py Outdated Show resolved Hide resolved
coremltools/models/utils.py Outdated Show resolved Hide resolved
coremltools/models/utils.py Show resolved Hide resolved
coremltools/models/utils.py Outdated Show resolved Hide resolved
coremltools/models/utils.py Outdated Show resolved Hide resolved
@TobyRoseman
Copy link
Collaborator

  • Please make sure to get a successful CI run before merging.

YifanShenSZ
YifanShenSZ previously approved these changes Jul 25, 2024
Copy link
Collaborator

@YifanShenSZ YifanShenSZ left a comment

Choose a reason for hiding this comment

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

Awesome utility!

coremltools/models/utils.py Outdated Show resolved Hide resolved
coremltools/models/utils.py Outdated Show resolved Hide resolved
coremltools/models/utils.py Show resolved Hide resolved
@jakesabathia2
Copy link
Collaborator Author

Can you also add a small section describing the util in the docs, along with its use case (when to use it, e.g. SD model thats big) : https://apple.github.io/coremltools/docs-guides/source/mlmodel-utilities.html

Done!

YifanShenSZ
YifanShenSZ previously approved these changes Jul 26, 2024
aseemw
aseemw previously approved these changes Jul 26, 2024
Copy link
Collaborator

@aseemw aseemw left a comment

Choose a reason for hiding this comment

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

PR looks good.
I think it would be good to merge, after addressing two more comments from my side:

  • a few changes in the documentation
  • If we can add another unit test which uses constexpr ops that would be great (take the uncompressed model, palletize it and then chunk it for example). In the original chunking script, I remember fixing a bunch of issues which only came up when we had used constexpr ops.

docs-guides/source/mlmodel-utilities.md Outdated Show resolved Hide resolved
docs-guides/source/mlmodel-utilities.md Outdated Show resolved Hide resolved
docs-guides/source/mlmodel-utilities.md Outdated Show resolved Hide resolved
@jakesabathia2
Copy link
Collaborator Author

If we can add another unit test which uses constexpr ops that would be great (take the uncompressed model, palletize it and then chunk it for example). In the original chunking script, I remember fixing a bunch of issues which only came up when we had used constexpr ops.

Done!

@jakesabathia2
Copy link
Collaborator Author

https://gitlab.com/coremltools1/coremltools/-/pipelines/1392495182
CI passes, except the test_pytorch_310 exceed time limit

@nighting0le01
Copy link

hi i think there is issue with this API when the model is quantized before chunking: #2320

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.

6 participants