-
Notifications
You must be signed in to change notification settings - Fork 15
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
Initial code for making python_env output an artifact #214
Initial code for making python_env output an artifact #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is good to merge with a few suggestions for addressing technical debt:
1. Code Modularization
- Break down large functions into smaller, more manageable pieces. For instance, the
run
method inmetadata/push.py
could be refactored into several helper methods for improved clarity and maintainability. - Consider splitting the
cmflib/cmf_server.py
file into multiple smaller files or a dedicated folder structure. For example, methods likelog_model_with_version
andlog_execution_metrics_from_client
could be moved to a separatelogs
file.
2. Type Annotations
- Adding type annotations to function signatures would improve the readability and maintainability of the code, as well as facilitate better type checking.
3. Logging
- Replace
print
statements with proper logging functionality. This would enhance the ability to track and debug issues in a production environment.
The PR looks good. I will suggest to add comments to make it more detailed before we merging. |
Created stories for these in our backlog @shreyas-kulkarni09 |
@rishabhsharma22 Commented all the changes i have done with respect to this story. If you feel need for more, please let me know. |
Related Issues / Pull Requests
List all related issues and/or pull requests if there are any.
Description
Include a brief summary of the proposed changes.
Environment
What changes are proposed in this pull request?
examples in this repository need to be updated too).
Checklist:
uses Google-style formatting and any other formatting that is supported by mkdocs and plugins this project
uses.