You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Right now the erfa.pyx (and erfa.py from #3141) are checked in as source code. These files are entirely auto-generated using cython_generator.py, so this seems wasteful of repository bandwidth in the event of future changes to the templates or other aspects of the erfa wrappers (incl. e..g, #3137).
Some discussion of this already has happened in #2992, #3134, and #3141 . Most relevant is an issue pointed out by @astrofrog, that this would then imply a compile-time dependency on jinja2. Presumably we could avoid this for releases by having the tarballs include the autogenerated source, just as we do right now with Cython-generated c code. I personally don't think it's a big deal to require jinja2 at compile-time, because it's already necessary for sphinx, which developers should all have to build the docs.
It also might make sense to include this on astropy_helpers in a slightly more generic form, allowing anycython_generator.py sort of usage. Or we could put it only in the core for now and later generalize if there's call for it.
@eteq - one could be even more generic and just have a 'pre-processing' step that could be called to generate any source before doing anything else. This could also be defined in setup_package.py, and there could be a special function name for preprocessing (e.g. preprocess()).
I wonder if it's really worth the effort to change that. The files change rarely and we are talking kb here not mb. That would be different if we did use cython_generartor.py in many places, but for now we can probably use our developer time better for other issues.
@eteq - if we go this route (and I do like it), one thing to check is that the output is the same with python2 and python3 (currently, python2 outputs unicode strings for the warnings, like u'OK', while python3 omits the u; see #3138).
@astrofrog - I like the idea of a generic pre-processing/generation step (although beware the condiment effect 😉). But I'm also fine with a stopgap measure that just works with erfa, given the fact that there are PRs flying around now that could use this feature.
@hamogu - I understand where you're coming from, but in fact in this case it is MBs... The generated erfa files are ~1MB of source code... That's why I think this is worth the effort now.
It's also part of my endless effort to keep developer overhead low: any step that requires running some tool before a commit is dangerous, because it doesn't always happen, so the generated and templates are sometimes out-of-sync. This is error-prone even for the people that write it, and this can be a huge time sink, especially for new contributors...
Activity
astrofrog commentedon Nov 30, 2014
@eteq - one could be even more generic and just have a 'pre-processing' step that could be called to generate any source before doing anything else. This could also be defined in
setup_package.py, and there could be a special function name for preprocessing (e.g.preprocess()).hamogu commentedon Nov 30, 2014
I wonder if it's really worth the effort to change that. The files change rarely and we are talking kb here not mb. That would be different if we did use
cython_generartor.pyin many places, but for now we can probably use our developer time better for other issues.mhvk commentedon Nov 30, 2014
@eteq - if we go this route (and I do like it), one thing to check is that the output is the same with python2 and python3 (currently, python2 outputs unicode strings for the warnings, like
u'OK', while python3 omits theu; see #3138).cdeil commentedon Dec 1, 2014
👍 to do other code-generation in addition to Cython using
jinja2if it's useful.astrofrog commentedon Dec 1, 2014
Just FYI I'm exploring ways we can support this with astropy-helpers
eteq commentedon Dec 1, 2014
@astrofrog - I like the idea of a generic pre-processing/generation step (although beware the condiment effect 😉). But I'm also fine with a stopgap measure that just works with erfa, given the fact that there are PRs flying around now that could use this feature.
@hamogu - I understand where you're coming from, but in fact in this case it is MBs... The generated erfa files are ~1MB of source code... That's why I think this is worth the effort now.
It's also part of my endless effort to keep developer overhead low: any step that requires running some tool before a commit is dangerous, because it doesn't always happen, so the generated and templates are sometimes out-of-sync. This is error-prone even for the people that write it, and this can be a huge time sink, especially for new contributors...
astrofrog commentedon Dec 2, 2014
@eteq - for an implementation of auto-generation starting from one of @jwoillez's branches, see #3170
astrofrog commentedon Jan 13, 2015
This is addressed by #3166