Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Ml for synthesis prototype - Refactoring #260
Ml for synthesis prototype - Refactoring #260
Changes from 6 commits
513ecd0
7fec9b2
6117beb
3f66495
44f471f
71a01cb
de72eb9
f0b91ab
fdd7ae6
69aef37
174ad21
0636193
3945599
f85e126
3601755
aeb2c04
f0941f3
c2d1de1
f268fb1
a93a304
83c68ab
ecd35f9
b2135a6
8e976f5
6d73b06
5cd88ac
47c7497
8a421b5
ffa9302
64edcc4
8306454
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
used env var name (
NITTA_RUN_COMMAND
) doesn't matchnitta/.github/workflows/ci.yml
Lines 246 to 247 in ecd35f9
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.
also it may be better to do it like
it's consistent with the
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.
Don't fully understand the idea behind
EnvVarNames
class, but consistency is an argument. Thank you!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 idea is quite boring, EnvVarNames.MODELS_DIR had to be reused in tests:
nitta/ml/synthesis/src/tests/test_smoke.py
Lines 80 to 86 in ffa9302
So I preferred to do it this way instead of hardcoding :)
Class is just for namespacing.
Regardless, I think it's good to have a single place where all the env var names (that python modules understand and use) are listed together, so they're not scattered across the code as literals. If there ever will be a need to generate docs for them, it'll make it simpler.