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

Make parsetab/lextab generation more automatic/maintainable #7177

Open
eteq opened this issue Feb 9, 2018 · 5 comments
Open

Make parsetab/lextab generation more automatic/maintainable #7177

eteq opened this issue Feb 9, 2018 · 5 comments

Comments

@eteq
Copy link
Member

eteq commented Feb 9, 2018

In #7174 the PLY lextab/parsetab tables were updated and some instructions were added on how to do this. But some of the discussion around it (in the PR and on slack) reveals that there are some complexities there. This issue is to build a plan for how we might go about making this more maintainable.

The current process is to delete the table files, re-build astropy inplace, and then run the tests in the part of the astropy code base that uses the tables. This is necessary because at least in units, the tables only get generated when the code using them is hit.

However, the table-building process is pretty fast. So some alternatives include:

  1. Not including the tables in the source code at all. Instead let them auto-generate at first use. I'm not clear on why we weren't doing this from the outset, actually, since we bundle PLY in extern.
  2. Generating them with a build command - e.g. python setup.py make_lexparsetabs or something. That will require adding some code or other annotation that figures out where all the table-generation code is. This is non-trivial right now because for coordinates you just have to create the _AngleParser object, but in units I think you actually have to generate the relevant unit (which requires importing all the various unit sub-packages).
  3. Have functions at module level that can be called via e.g. from astropy.coordinates import make_parselextabs;make_parselextabs() or similar. Still requires most of the items from 2 above, though.

cc @astrofrog @saimn @bsipocz

@eteq eteq changed the title Make parsetab/lextab generation more automatic Make parsetab/lextab generation more automatic/maintainable Feb 9, 2018
@saimn
Copy link
Contributor

saimn commented Feb 9, 2018

Having a written (reproducible) procedure to regenerate the files would be useful indeed, as it is not clear which imports are needed to trigger each file generation. For me a "quick and not too dirty" script would be good enough (it could remove the lextab/parsetab files and do the needed imports).

@eteq
Copy link
Member Author

eteq commented Feb 14, 2018

OK, I think I've experimented enough with ply that I now know how a script like @saimn is suggesting could be written.

But the question still stands if it's desirable - that is, if we should stop packaging them with the source code at all. It's even easier to make that change than making the script actually: basically, delete the files from git and we are done - they auto-generate at import-time. And my testing says the time to generate the tables is trivial (ms), so a user likely won't notice at all. (That could change if the tables become much more complicated... but that's premature optimization.)

(I guess I'm slowly convincing myself that's what we should do...)

@astrofrog
Copy link
Member

Is there a reason why we didn't want to rely on auto-generation originally? Performance?

@astrofrog
Copy link
Member

astrofrog commented Feb 14, 2018

Relevant commits/issues for my question:

ef839a1#diff-d33d630835d502d02afae8a94d37f7ce

In particular the following comment in #1112

The problem is that the parsetab files need to be generated at install time too, and currently that isn't possible in Python 3 due to SyntaxErrors when trying to import them.

This is fixed now though. I guess one question is whether in e.g. root installs of Astropy the files could not be generated due to permission errors? We'd have to make sure they are generated at install time.

@eteq
Copy link
Member Author

eteq commented Feb 21, 2018

Agreed that install-time (or probably build-time) is the best way to go @astrofrog. I'll assign this to myself, as I spent some time last week figuring out how all of this actually works. (Soon enough I'll be saying things like #4318 (comment) 😉)

@eteq eteq self-assigned this Feb 21, 2018
@eteq eteq added this to the v3.1 milestone Feb 21, 2018
@bsipocz bsipocz removed this from the v3.1 milestone Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants