-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix handling of script arguments in tracer #15518
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.
LGTM !
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.
It is probably best if we make self.script_args and params both lists in the future. It seems unintuitive to be passing two different data types. The user would do the conversion outside in reverse to what we do internally.
This looks like undocumented behavior and we can probably change it, but it may break some apps. |
* Don't assume script args start with double dash * add changelog Co-authored-by: Luca Antiga <[email protected]> Co-authored-by: awaelchli <[email protected]> (cherry picked from commit 5cf5036)
* Don't assume script args start with double dash * add changelog Co-authored-by: Luca Antiga <[email protected]> Co-authored-by: awaelchli <[email protected]> (cherry picked from commit 5cf5036)
What does this PR do?
TracerPythonScript
assumes that additionalparams
start with--
For instance, passing
params={"a": 1}
is turned intoscript_args == "--a=1"
. This breaks when this assumption is not true, for instance when parsing arguments using hydra.ccThis PR removes this assumption. The (unavoidable) downside is a
params
argument needs to be provided as{"--a": 1}
if it is meant to be turned into--a=1
, otherwise it is converted toa=1
.Note that this is needed for Lightning-Universe/Training-Studio_app#216 to work
Fixes #<issue_number>
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃
cc @Borda @justusschock