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

tensorflow model: fix TensorflowModelMixin usage #201

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

antoinejeannot
Copy link
Contributor

Hi,

Following the discussions around MRO, I noticed an issue with regards to the way we use the TensorflowModelMixin.

  • Before: its initialization is never called, hence theImportError if tensorflow is missing is never raised and a cryptic error is raised later on

  • After this PR: its initialization is called, and directly raises the ImportError if tensorflow is missing.

Thanks for your review

mathilde-leval
mathilde-leval previously approved these changes Oct 11, 2023
Copy link
Contributor

@mathilde-leval mathilde-leval left a comment

Choose a reason for hiding this comment

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

LGTM

ldeflandre
ldeflandre previously approved these changes Oct 12, 2023
Copy link
Contributor

@ldeflandre ldeflandre left a comment

Choose a reason for hiding this comment

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

Nice work

modelkit/core/models/tensorflow_model.py Outdated Show resolved Hide resolved
@antoinejeannot
Copy link
Contributor Author

PTAL @ldeflandre

@antoinejeannot antoinejeannot merged commit 139af52 into Cornerstone-OnDemand:main Nov 13, 2023
13 checks passed
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