Skip to content

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Sep 10, 2019

Converts Jinja macros to native Python. Uses a functional style to generate CircleCI workflow members.

@kostmo kostmo force-pushed the u/kostmo/readability-experiment branch from 47b9ec2 to fd36e82 Compare September 10, 2019 06:50
@codecov-io
Copy link

codecov-io commented Sep 10, 2019

Codecov Report

Merging #1321 into master will increase coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1321      +/-   ##
==========================================
+ Coverage   65.89%   66.36%   +0.47%     
==========================================
  Files          75       75              
  Lines        5785     5944     +159     
  Branches      885      934      +49     
==========================================
+ Hits         3812     3945     +133     
- Misses       1708     1730      +22     
- Partials      265      269       +4
Impacted Files Coverage Δ
torchvision/ops/roi_align.py 65.15% <0%> (-6.28%) ⬇️
torchvision/ops/roi_pool.py 69.49% <0%> (-4.59%) ⬇️
torchvision/extension.py 41.66% <0%> (+0.75%) ⬆️
torchvision/transforms/functional.py 72.6% <0%> (+1.34%) ⬆️
torchvision/transforms/transforms.py 83.89% <0%> (+2.32%) ⬆️
torchvision/utils.py 70.17% <0%> (+4.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a129b6b...3e3a495. Read the comment docs.

@kostmo kostmo force-pushed the u/kostmo/readability-experiment branch from fd36e82 to 33c6faf Compare September 12, 2019 18:46
@kostmo kostmo changed the title [WIP] readability experiment Adopt native-Python code generation convention Sep 12, 2019
@kostmo kostmo changed the title Adopt native-Python code generation convention [WIP] Adopt native-Python code generation convention Sep 12, 2019
@kostmo kostmo marked this pull request as ready for review September 12, 2019 19:39
@kostmo kostmo force-pushed the u/kostmo/readability-experiment branch 2 times, most recently from bf35a3f to 9a513d3 Compare September 12, 2019 20:29
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

I think this is more readable than the macros, but I'd like to see the guidelines for when to add the declarations in the py instead of the yaml.in file.

should be at the top of the file for easy updating.

See this document for design rationale:
https://fb.quip.com/zxJaAMOrNZ2S
Copy link
Member

Choose a reason for hiding this comment

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

I don't have access to this document, would it make sense to put it in a public link, like a gist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, posted below.

@kostmo
Copy link
Member Author

kostmo commented Sep 12, 2019

Shall all/most code generation logic live in the Jinja template?

TL;DR: No.

Currently, we use code generators to produce parts of the CircleCI YAML config files. I will be continuing this course in the pytorch/builder repo for integration tests. I would like to establish a convention going forward regarding generation practices.

One important objective is to minimize complexity of the YAML generation logic. In fact, when possible, code generation should be avoided in favor of utilizing CircleCI's API in static YAML. However, generating a matrix of configs is one use case where we cannot currently avoid code generation.

Some initial demonstrations of using exclusively Jinja as a code generator were created. I like Jinja as a means for substituting variables and simple looping. However, personally the appeal of Jinja templating ends at macros; to me they look a lot like Python functions but with inferior ergonomics to Python (e.g. more syntactical overhead, no automatic linting in CI, absent syntax highlighting in editors/Github).

I've put up a side-by-side comparison of Jinja macros and equivalent native Python for a simple YAML generation example. I also made a Python alternative using a class for the sake of experiment.

There is a contention that storing the logic strictly in Jinja templates will help help establish boundaries for complexity, whereas for general purpose code (i.e. native Python), you would have to write some rules down about limiting complexity.

However, I contend that the upper bound on complexity imposed by Jinja is loose; the template language is still sufficiently rich to get into trouble. It doesn't seem to get around the need for writing down rules about acceptable usage.

Another rationale given for Jinja-only logic is to have all the logic in one file, making it easy to skim over and read in its totality. However, I'd contend that given equivalent code length, storing logic in one file vs. two files is merely a choice between two comparable navigation modes: scrolling up and down vs. flipping left/right through editor tabs (or even displaying them side-by-side).

A couple other advantages of using the PyYAML library are data composability (which allows correct indentation to be handled automatically) and automatic quoting of YAML values when required.

Finally, regarding the proliferation of languages in CI infra: we can reasonably expect PyTorch devs to be comfortable with Python code, but devs will be strictly less familiar with Jinja syntax and capabilities.


Conclusion: Internal consensus is that Jinja macros are not very readable, and that logic should be expressed in native Python, while exercising restraint toward dynamic config generation.

use flake8 with Python 3 on .circleci directory
@kostmo kostmo force-pushed the u/kostmo/readability-experiment branch from 9a513d3 to 3e3a495 Compare September 12, 2019 22:42
@kostmo kostmo requested review from ezyang and mingbowan September 12, 2019 23:02
@kostmo kostmo changed the title [WIP] Adopt native-Python code generation convention Adopt native-Python code generation convention Sep 12, 2019
@mingbowan mingbowan merged commit 6de158c into master Sep 12, 2019
@fmassa fmassa deleted the u/kostmo/readability-experiment branch September 13, 2019 14:50
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
Compare to application to vision repo in pytorch#1321
See rationale writeup: pytorch/vision#1321 (comment)
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
kostmo added a commit to kostmo/audio that referenced this pull request Dec 27, 2019
vincentqb pushed a commit to pytorch/audio that referenced this pull request 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.

4 participants