Skip to content

Skip generating generic unit parsing tables - #5563

Merged
eteq merged 1 commit into
astropy:masterfrom
bsipocz:units_skip_generating_parsingtable
Dec 9, 2016
Merged

Skip generating generic unit parsing tables#5563
eteq merged 1 commit into
astropy:masterfrom
bsipocz:units_skip_generating_parsingtable

Conversation

@bsipocz

@bsipocz bsipocz commented Dec 6, 2016

Copy link
Copy Markdown
Member

Upgrading the external PLY package to version 3.9 somehow broke the parsing table parsing. Running the tests with python2 now always overwrites the existing tables (as pointed out here #5526 (comment)). The tables in master were generated and then treated by clean_parse_tables.py at the time of the PLY upgrade.

There are 4 sets of these parsing tables, but it only effects units.format.generic, as it's somehow accessed early during the setup process (?). I couldn't pin point the exact location, but braking the _parse() classmethod gives the following traceback below.

Anyhow, since none of the code in this traceback found to be changed recently, my workaround to fix this asap would be to revert the removal of the try/except from 9ee8d3c only for this file only (as the 3 others are still working as expected).

cc @taldcroft and @mhvk

Traceback (most recent call last):
  File "setup.py", line 48, in <module>
    package_info = get_package_info()
  File "/Users/bsipocz/munka/devel/astropy/astropy_helpers/astropy_helpers/setup_helpers.py", line 396, in get_package_info
    ext_modules.extend(setuppkg.get_extensions())
  File "astropy/wcs/setup_package.py", line 252, in get_extensions
    generate_c_docstrings()
  File "astropy/wcs/setup_package.py", line 126, in generate_c_docstrings
    from astropy.wcs import docstrings
  File "/Users/bsipocz/munka/devel/astropy/astropy/wcs/__init__.py", line 30, in <module>
    from .wcs import *
  File "/Users/bsipocz/munka/devel/astropy/astropy/wcs/wcs.py", line 50, in <module>
    from ..io import fits
  File "/Users/bsipocz/munka/devel/astropy/astropy/io/fits/__init__.py", line 64, in <module>
    from . import convenience
  File "/Users/bsipocz/munka/devel/astropy/astropy/io/fits/convenience.py", line 72, in <module>
    from ...units.format.fits import UnitScaleError
  File "/Users/bsipocz/munka/devel/astropy/astropy/units/__init__.py", line 20, in <module>
    from . import astrophys
  File "/Users/bsipocz/munka/devel/astropy/astropy/units/astrophys.py", line 46, in <module>
    def_unit(['lyr', 'lightyear'], (_si.c * si.yr).to(si.m),
  File "/Users/bsipocz/munka/devel/astropy/astropy/constants/constant.py", line 45, in wrapper
    self.unit.to(inst.unit)
  File "/Users/bsipocz/munka/devel/astropy/astropy/units/quantity.py", line 757, in unit
    return self._unit
  File "/Users/bsipocz/munka/devel/astropy/astropy/utils/decorators.py", line 722, in __get__
    val = self.fget(obj)
  File "/Users/bsipocz/munka/devel/astropy/astropy/constants/constant.py", line 165, in _unit
    return Unit(self._unit_string)
  File "/Users/bsipocz/munka/devel/astropy/astropy/units/core.py", line 1836, in __call__
    raise ValueError(msg)
ValueError: 'm / (s)' did not parse as unit: type object 'Generic' has no attribute '_parser'

@pllim pllim added the units label Dec 6, 2016
@mhvk

mhvk commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

This seems like a good change, but I've never really understood those tables. cc @mdboom in case he can still comment...

@eteq

eteq commented Dec 7, 2016

Copy link
Copy Markdown
Member

Am I understanding right that this happens only when running the tests, @bsipocz, not in a normal runtime situation?

@bsipocz

bsipocz commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

@eteq - Actually I haven't tested what it does for setup.py develop or install, but it definitely happens for running the tests, albeit only on python2.

@bsipocz

bsipocz commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

@eteq - Yes, it does happen for python2 setup.py develop, too, running it on master I got the two files modified:

	modified:   astropy/units/format/generic_lextab.py
	modified:   astropy/units/format/generic_parsetab.py

@mhvk

mhvk commented Dec 7, 2016

Copy link
Copy Markdown
Contributor

This is still most confusing. I also noticed your change here essentially reverts changes made in #4003. It would be good to understand why the lexer thinks it has to regenerate the file (on python2, but not 3).

@bsipocz

bsipocz commented Dec 7, 2016

Copy link
Copy Markdown
Member Author

@mhvk - Yes, I mentioned the revert in the description. The more puzzling question why the lexer of the previous version was happy with the file even though that also went through the cleanup script. Someone who knows the building procedure may be able to answer these.

@eteq eteq added Affects-dev PRs and issues that do not impact an existing Astropy release Bug no-changelog-entry-needed labels Dec 7, 2016
@eteq eteq added this to the v1.3.0 milestone Dec 9, 2016
@eteq

eteq commented Dec 9, 2016

Copy link
Copy Markdown
Member

I can confirm now that this fixes things. But it's a but concerning we don't understand why. I think the best thing is to merge this now and figure out the underlying cause later - will make a separate issue for that.

@eteq
eteq merged commit 55053c4 into astropy:master Dec 9, 2016
eteq added a commit that referenced this pull request Dec 9, 2016
@eteq
Merge pull request #5563 from bsipocz/units_skip_generating_parsingtable
1374036 
Skip generating generic unit parsing tables
@eteq eteq mentioned this pull request Dec 9, 2016
Closed
@eteq

eteq commented Dec 9, 2016

Copy link
Copy Markdown
Member

backported to v1.3.x in 1374036

@saimn saimn mentioned this pull request Jan 23, 2017
Merged
@bsipocz
bsipocz deleted the units_skip_generating_parsingtable branch September 16, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release Bug no-changelog-entry-needed units

Projects

None yet

Milestone

v1.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@bsipocz @mhvk @eteq @pllim