-
Notifications
You must be signed in to change notification settings - Fork 674
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
Adopt native-Python code generation convention #378
Conversation
220c15e
to
f559241
Compare
# XXX This logic is suspect... | ||
upload_job_filter_branch = not is_py3_linux and filter_branch |
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.
Not sure if this was a bug introduced at some point in the in the existing logic. Currently, the filters:
yaml chunk is excluded from *_upload
jobs on Linux with Python 3, so I've crafted my logic to match this behavior. Not sure what the rationale is, though, or whether that's intentional.
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.
Is this code generation (with exception) the same across domains? What are the differences?
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.
There are some minor differences with the vision
repo's config.yml
file, and by extension the codegen. To name a few:
vision
exercises an extra dimension of configuration with different versions of CUDA + cpu- it does not have
smoke_test
jobs - it lacks
filters:
sections in the YAML
.circleci/sort-yaml.py
Outdated
@@ -0,0 +1,13 @@ | |||
#!/usr/bin/env python3 | |||
|
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 have added this little script to prove that my change yields identical output, modulo ordering of YAML keys, whitespace, and quoting style (single- vs. double-quotes).
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.
So this is a test file, right? How about adding it as .circleci/test/test_sort_yaml.py
?
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.
Will do, though it shouldn't be considered a standalone test, but rather a utility to facilitate a manual comparison for this somewhat extensive code change.
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.
Tests are failing, see here and below
./.circleci/sort-yaml.py:11:11: E401 multiple imports on one line
./.circleci/regenerate.py:37:95: E999 SyntaxError: invalid syntax
.circleci/sort-yaml.py
Outdated
@@ -0,0 +1,13 @@ | |||
#!/usr/bin/env python3 | |||
|
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.
So this is a test file, right? How about adding it as .circleci/test/test_sort_yaml.py
?
# XXX This logic is suspect... | ||
upload_job_filter_branch = not is_py3_linux and filter_branch |
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.
Is this code generation (with exception) the same across domains? What are the differences?
f559241
to
508522f
Compare
Closes pytorch#304 See rationale writeup: pytorch/vision#1321 (comment)
508522f
to
0ef4742
Compare
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, thanks!
Compare to application to vision repo in pytorch/vision#1321
See rationale writeup: pytorch/vision#1321 (comment)
Fixes #304