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

Make parsing parameter from CLI work properly when it is a list #128

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

dangquangdon
Copy link
Contributor

@dangquangdon dangquangdon commented Mar 18, 2024

Let argparser knows how to properly parse the parameter of type list by providing a callback that split the parameter using comma as default.

@dangquangdon dangquangdon requested review from a team, akx and memona008 and removed request for a team March 18, 2024 12:09
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Can we have a test for this, please?

AFAIK nargs="+" will assume the user enters the argument as

$ python foo.py --my-list-argument a b c d --other-argument blah

so a test would at least demonstrate that that's the way to use this feature.

@dangquangdon dangquangdon marked this pull request as draft March 18, 2024 17:52
@dangquangdon dangquangdon force-pushed the dangquangdon-properly-parse-list-parameter-from-cli branch 3 times, most recently from abb58b0 to 7595c81 Compare March 20, 2024 18:05
@dangquangdon dangquangdon marked this pull request as ready for review March 20, 2024 18:07
@dangquangdon
Copy link
Contributor Author

@akx I changed the approach and also described it in the PR description

@dangquangdon dangquangdon requested a review from akx March 20, 2024 18:12
@akx akx marked this pull request as draft March 21, 2024 09:10
@akx
Copy link
Member

akx commented Mar 21, 2024

I emdraftened this – I think we need to design this new stepconfig.json thing a little more first since it'd be forever a part of the /valohai/ mount structure (and would need to eventually be documented like the others at https://docs.valohai.com/hc/en-us/articles/18704309491473-System-Configuration-Files and so on).

@dangquangdon
Copy link
Contributor Author

I emdraftened this – I think we need to design this new stepconfig.json thing a little more first since it'd be forever a part of the /valohai/ mount structure (and would need to eventually be documented like the others at https://docs.valohai.com/hc/en-us/articles/18704309491473-System-Configuration-Files and so on).

Good to know that we have to document it publicly 👍 . My idea is stepconfig.json is simply just the information of the step defined in valohai.yaml 🤔

@dangquangdon dangquangdon force-pushed the dangquangdon-properly-parse-list-parameter-from-cli branch from 7595c81 to fd688c9 Compare March 26, 2024 09:33
@dangquangdon
Copy link
Contributor Author

@akx I reduce the scope of this PR now to only parse the parameter's value using comma as default. We will make further changes once we find out a good way to get multiple-separator from peon and roi

@dangquangdon dangquangdon marked this pull request as ready for review March 26, 2024 09:36
When the values of a parameter are provided via CLI, split them
into a list properly with the default separator is a comma
@dangquangdon dangquangdon force-pushed the dangquangdon-properly-parse-list-parameter-from-cli branch from fd688c9 to 20fe008 Compare April 22, 2024 05:50
@dangquangdon dangquangdon requested a review from akx April 22, 2024 05:50
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

👍

@akx akx merged commit 88d89a5 into master Apr 22, 2024
5 checks passed
@akx akx deleted the dangquangdon-properly-parse-list-parameter-from-cli branch April 22, 2024 05:57
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.

2 participants