Add Oncoclear project focused on FastAPI logic #194
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
marwan37
reviewed
Apr 14, 2025
Comment on lines
+49
to
+75
| else: | ||
| logger.info(f"Model promoted to {stage}!") | ||
| is_promoted = True | ||
|
|
||
| # Get the model in the current context | ||
| current_model = get_step_context().model | ||
|
|
||
| # Get the model that is in the production stage | ||
| client = Client() | ||
| try: | ||
| stage_model = client.get_model_version( | ||
| current_model.name, stage | ||
| ) | ||
| # We compare their metrics | ||
| prod_accuracy = ( | ||
| stage_model.get_artifact("sklearn_classifier") | ||
| .run_metadata["test_accuracy"] | ||
| ) | ||
| if float(accuracy) > float(prod_accuracy): | ||
| # If current model has better metrics, we promote it | ||
| is_promoted = True | ||
| current_model.set_stage(stage, force=True) | ||
| except KeyError: | ||
| # If no such model exists, current one is promoted | ||
| is_promoted = True | ||
| current_model.set_stage(stage, force=True) | ||
| return is_promoted |
Contributor
There was a problem hiding this comment.
nitpick comment: you're already reassigning is_promoted to True at the beginning of the else block, so the identical reassignments below are redundant.
Contributor
Author
There was a problem hiding this comment.
I actually like it that way to make it explcit
marwan37
reviewed
Apr 14, 2025
oncoclear/steps/model_trainer.py
Outdated
| model_type: str = "sgd", | ||
| target: Optional[str] = "target", | ||
| ) -> Annotated[ | ||
| ClassifierMixin, ArtifactConfig(name="sklearn_classifier", is_model_artifact=True) |
Contributor
There was a problem hiding this comment.
since artifact_type will be deprecated soon, you can update the function's return type to use the new annotation style
Annotated[
ClassifierMixin, ArtifactConfig(name="sklearn_classifier", artifact_type=ArtifactType.MODEL)
]:
marwan37
approved these changes
Apr 14, 2025
Contributor
marwan37
left a comment
marwan37
left a comment
There was a problem hiding this comment.
Other than the 2 minor comments, everything else looks great 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just need to add images now
How to Run:
Detailed instructions are available in the
README.md, covering setup, dependency installation, ZenML integration installation, and commands to execute each pipeline.Goal:
To establish the foundational codebase for the
oncoclearproject, providing a robust example of a ZenML-powered MLOps workflow for a standard classification task.