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

Letting users pass args via M_* #328

Merged
merged 2 commits into from
Jan 13, 2024
Merged

Conversation

rwitten
Copy link
Collaborator

@rwitten rwitten commented Jan 12, 2024

Now you can pass args via setting an env var M_ARG_NAME.

@rwitten rwitten force-pushed the rwitten_add_env_vars branch from 09757be to ba7d6ff Compare January 12, 2024 05:09
@rwitten rwitten changed the title Letting users pass args via MAX_* Letting users pass args via M_* Jan 12, 2024
@@ -61,9 +65,17 @@ def _lists_to_tuples(l: list[Any]) -> Union[tuple[Any],list[Any]]:

class _HyperParameters():
# pylint: disable=missing-class-docstring
def _validate_env_variables(self, raw_data_from_yaml):
for environment_var in os.environ:
if environment_var[:4] == _MAX_PREFIX:
Copy link
Collaborator

@raymondzouu raymondzouu Jan 12, 2024

Choose a reason for hiding this comment

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

I think this is supposed to be environment_var[:2] to check for "M_". Could also change it to environment_var[:len(_MAX_PREFIX)] in case the prefix is changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes thank you Raymond, fixed on new revision. I changed the name late. Nice eagle eyes calling out both the unnecessary constant and the wrong constant!

(p.s. -- I woke up at midnight because I dreamt of this bug. amazing that you had already caught it when I woke up!)

@rwitten rwitten force-pushed the rwitten_add_env_vars branch from ba7d6ff to 633847b Compare January 12, 2024 16:50
@rwitten rwitten force-pushed the rwitten_add_env_vars branch from 633847b to 18d2469 Compare January 12, 2024 16:54
@copybara-service copybara-service bot merged commit b7daa82 into main Jan 13, 2024
7 checks passed
@copybara-service copybara-service bot deleted the rwitten_add_env_vars branch January 13, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants