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

Adopt native-Python code generation convention #378

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Dec 27, 2019

Compare to application to vision repo in pytorch/vision#1321
See rationale writeup: pytorch/vision#1321 (comment)

Fixes #304

@kostmo kostmo force-pushed the adopt-native-codegen branch 4 times, most recently from 220c15e to f559241 Compare December 27, 2019 05:36
Comment on lines +45 to +51
# XXX This logic is suspect...
upload_job_filter_branch = not is_py3_linux and filter_branch
Copy link
Member Author

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.

Copy link
Contributor

@vincentqb vincentqb Dec 27, 2019

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?

Copy link
Member Author

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

@@ -0,0 +1,13 @@
#!/usr/bin/env python3

Copy link
Member Author

@kostmo kostmo Dec 27, 2019

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).

Copy link
Contributor

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 ?

Copy link
Member Author

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.

Copy link
Contributor

@vincentqb vincentqb left a 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

@@ -0,0 +1,13 @@
#!/usr/bin/env python3

Copy link
Contributor

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 ?

.circleci/config.yml Show resolved Hide resolved
Comment on lines +45 to +51
# XXX This logic is suspect...
upload_job_filter_branch = not is_py3_linux and filter_branch
Copy link
Contributor

@vincentqb vincentqb Dec 27, 2019

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?

@kostmo kostmo force-pushed the adopt-native-codegen branch from f559241 to 508522f Compare December 27, 2019 17:52
Copy link
Contributor

@vincentqb vincentqb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@vincentqb vincentqb merged commit 42ffaf6 into pytorch:master Dec 27, 2019
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.

Apply "Adopt native-Python code generation convention" to this repo
2 participants