Repository navigation
PLY generated table handling is a pain #1112
Description
I noticed just now that a recent change (I think #1050 ? It seems to be PLY-related, anyway, @mdboom) led to astropy now producing a parsetab.py file that gets installed into the base namespace. So we would namespace-collide with any existing parsetab package or module, which presumably is a very bad thing (TM).
So we should figure out how to move parsetab.py back into astropy/extern/* where it belongs.
Activity
embray commentedon May 19, 2013
Okay. I've seen that happening too. Glad I'm not going crazy. This probably needs to be fixed before we release 0.2.2 :(
Make sure PLY doesn't write any files to the Astropy source tree. Fixes
astrofrog commentedon May 19, 2013
@eteq @iguananaut - see #1113 for a possible fix
taldcroft commentedon May 19, 2013
Yes, I was wondering where those parsetab.py files were coming from! I guess there is a reason I should be cautious doing "production" work with astropy master.
embray commentedon May 19, 2013
I've got a solution to this that works rather well EXCEPT in Python 3. 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.
For now we could just disable the optimizations for Python 3. Or I could come up with a slightly more involved reorg.
I think that for v0.2.2, in the interest of getting the release out the door, I'll go with just including the generated files in the repo since they don't need to updated anyways. Thoughts?
embray commentedon May 19, 2013
For anyone who's interested, here's what I've got so far: https://github.com/iguananaut/astropy/compare/issue-1112
Basically, the PLY lex and yacc interfaces are a little kludgy at the moment (surprising considering who wrote it) but they do both accept passing in a module object for lextab and parsetab modules. So if they already exist we can just import them and pass them to
lex.lex()andyacc.yacc(). Otherwise we just provide those functions the name we want for the modules and what directory they should be output to, and it will generate them (making them importable next time).This works fine, but for the purpose of generating them at install time I'm still not sure what best to do :/
eteq commentedon May 20, 2013
@iguananaut - I think this may have been what you were saying in #1113, but can we just provide them in
astropy/extern/plyin the source code, or doesparsetabsget generated even if we have it already there? I guess from your diff I don't understand what's controlling what gets generated at install time (or are you saying you're not sure either?)mdboom commentedon May 20, 2013
Yeah -- the way this is all done is a terrible kludge. Sorry I hadn't noticed the parsetab thing when I was first evaluating PLY.
Personally, I'm fine with just not generating the files. We already take care to not create the parsers unless they actually get used (i.e. not on import time), and the construction time (since our grammars are not enormous) isn't really that bad. That just generally seems safer to get something out the door now.
Then for 0.3, we could consider doing something like @iguananaut's branch.
@eteq: To answer your question about putting these in
astropy/extern/ply: Each parser needs its own table, and we have three of them (for now), so these tables should go next to the parser implementation, not inastropy/extern/ply.embray commentedon May 20, 2013
It does get generated at import time depending on what you're importing. No, nothing happens if you
import astropy, but soon as youimport astropy.unitsfor example at least one of them gets generated.As a short-term fix I'm more toward leaving the generated files in the repository. They're small and easy enough to update, and that won't be much of an issue for 0.2.2 anyways.
mdboom commentedon May 20, 2013
@iguananaut: Oh, yes, I guess that's true since we parse some units always anyway...
Storing them in the repo is fine -- and that certainly solves the use case when installing to a read-only location (such as
/usr).embray commentedon May 20, 2013
Exactly. I think for now I'll leave this issue open, but put the files in the repo as a temporary fix.
As a temporary solution for astropy#1112, add the lex/parsetab module…
astrofrog commentedon May 20, 2013
Should this issue be reported upstream btw? (even though it's not really a bug, just sub-optimal behavior)
embray commentedon May 20, 2013
No, I don't see why. I mean, the behavior is documented. I guess it could be done better but I don't have any specific ideas in mind for how to improve it.
Merge pull request #1116 from iguananaut/issue-1112-temp
Merge pull request #1116 from iguananaut/issue-1112-temp
eteq commentedon Jun 11, 2013
@iguananaut - so what needs to be imporved on from #1116 before this can be closed? Is the main thing figuring out how to auto-generate the grammars instead of having them in the source tree?
embray commentedon Jun 28, 2013
I believe that was it, but it's a huge hassle and I'm not inclined to prioritize that right now. I think there's some discussion going on in #1202 about making the code compatible across Python 2/3 which would make things a lot easier though.
mdboom commentedon Sep 20, 2013
So, this whole way that PLY generates its files is annoying, and I think we're keeping this issue open to address that at some point. But the title of the issue is no longer really true, thanks to #1116. Renaming issue.
bsipocz commentedon Dec 2, 2016
@eteq - I was totally new to the parsing tables, but with ~10mins of digging around I could figure out how to update them along with the updating the ply library. Since we only need to do this once in every ~2 years, probably this issue can be close now?
bsipocz commentedon Feb 9, 2017
Exercising my newly minted rights, I'm closing this as an issue that will probably be never addressed. As a temp solution the files got included in the repo, and as I said in the previous comment, figuring out how that works is not that difficult.
Please reopen if you think we need another solution.