-
Notifications
You must be signed in to change notification settings - Fork 78
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
RHOAIENG-11216: Add script to automate the creation of a new Python-based image from an existing one #672
RHOAIENG-11216: Add script to automate the creation of a new Python-based image from an existing one #672
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.
I put some comments, but LGTM in general!
@jiridanek Please let me know when you finish with the review. |
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.
Guess I'm done. My main complaint is probably that there's huge amount of comments even for things that are fairly obvious. Hopefully if you switch to using python type annotations, you'd feel less need to comment every parameter of a 3 line function, where it's equally easy to read the implementation body as to read the comment.
I would not be offended if you decide to keep the comments, but please remove the comment from main(), that's way too much ;P
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.
Added comments about two aspects of type hints
- List, tuple, dict are parametric types, you can specify "list of what"
- When a function does not return anything, give it
-> None:
type annotation
786c6d0
to
1715d6d
Compare
/lgtm Thank you @caponetto, no more comments from my side 🙂 |
/lgtm me too |
Thanks for the review guys! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caponetto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/override ci/prow/rocm-notebooks-e2e-tests |
@caponetto: Overrode contexts on behalf of caponetto: ci/prow/images, ci/prow/rocm-notebooks-e2e-tests In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
https://issues.redhat.com/browse/RHOAIENG-11216
Description
Adding a Python script that generates a new folder structure for a Python-based project by copying an existing one and updating Python version references. It performs the following steps:
pipenv lock
to regenerate lock files based on the new Python version.If
pipenv lock
encounters errors, manual intervention is required to resolve compatibility issues between dependencies. Review the errors reported bypipenv
and adjust the dependencies as needed.See the
README
file for examples.How Has This Been Tested?
Manual executions, comparing the results with #659.
Merge criteria: