Skip to content
This repository was archived by the owner on Jun 3, 2025. It is now read-only.

Conversation

@markurtz
Copy link
Member

No description provided.

@markurtz markurtz requested a review from a team October 15, 2021 15:29
@markurtz markurtz self-assigned this Oct 15, 2021
@markurtz markurtz requested review from bfineran, mgoin and natuan and removed request for a team October 15, 2021 15:29
Copy link
Contributor

@bfineran bfineran left a comment

Choose a reason for hiding this comment

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

good catch on this necessary functionality. I'm a little worried about how coupled the attempt_num logic is in detect_framework with the execute function. Is there a specific reason for doing it this way? Any thoughts on having a detect function that returns all possible frameworks for an item, and then having execute return the first valid in-framework run of the function between the possible frameworks?

@markurtz
Copy link
Member Author

good catch on this necessary functionality. I'm a little worried about how coupled the attempt_num logic is in detect_framework with the execute function. Is there a specific reason for doing it this way? Any thoughts on having a detect function that returns all possible frameworks for an item, and then having execute return the first valid in-framework run of the function between the possible frameworks?

The original reason why I went this route is that I'm worried about performance for evaluating of a model. Evaluation of model could involve full loading of the model to check if it is tensorflow or keras for example. It is a very fair point that this pathway reduces readability and adds complexity. This is additionally a preoptimization for a performance use case we haven't hit yet, so I'll migrate the code over to return a list of frameworks instead of this more complicated pathway for now

bfineran
bfineran previously approved these changes Oct 19, 2021
Copy link
Contributor

@bfineran bfineran left a comment

Choose a reason for hiding this comment

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

LGTM. If we see performance issues when trying to load large models in the wrong framework, let's definitely update the detect_frameworks function to use what you had before, or maybe a generator if that would work

@markurtz markurtz merged commit 4fe5e44 into main Oct 19, 2021
markurtz added a commit that referenced this pull request Oct 19, 2021
* Add framework fallback ability to execute in sparseml

* remove unused variable

* decrease complexity of falling back on framework execution

* quality and test fixes

* update docs

* fix tests
markurtz added a commit that referenced this pull request Oct 19, 2021
* Add framework fallback ability to execute in sparseml

* remove unused variable

* decrease complexity of falling back on framework execution

* quality and test fixes

* update docs

* fix tests
@jeanniefinks jeanniefinks deleted the sparseml-execute-fallback branch February 10, 2022 18:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants