Skip to content

BUG: Fix loading parser tabs on pyc-only installations - #16159

Merged
mhvk merged 1 commit into
astropy:mainfrom
lpsinger:lex-pyc
Mar 7, 2024
Merged

BUG: Fix loading parser tabs on pyc-only installations#16159
mhvk merged 1 commit into
astropy:mainfrom
lpsinger:lex-pyc

Conversation

@lpsinger

@lpsinger lpsinger commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

According to the Python module loading
flow chart, when evaluating import foo and foo.py is not found, Python then reads foo.pyc.

One can take advantage of this fact to strip source files and leave only Python bytecode files for deployment in space-constrained execution environments such as AWS Lambda (see, for example, nasa-gcn/gcn.nasa.gov#2040).

Fix a bug in the wrappers for the lex and yacc wrappers that are used for parsing Astropy units so that they work on pyc-only installations.

Description

This pull request is to address ...

Fixes #

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

github-actions Bot commented Mar 5, 2024

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions github-actions Bot added the utils label Mar 5, 2024
@pllim pllim added this to the v6.1.0 milestone Mar 5, 2024
@pllim
pllim requested a review from mhvk March 5, 2024 14:30
@pllim

pllim commented Mar 5, 2024

Copy link
Copy Markdown
Member

Thanks!

This is subtle behavior change, so would need a change log, but I cannot decide between "api" within units or "other" on top level. Maybe Marten knows. 🤔

@pllim pllim mentioned this pull request Mar 5, 2024
Closed
@lpsinger lpsinger mentioned this pull request Mar 5, 2024
Merged
@eerovaher

eerovaher commented Mar 5, 2024

Copy link
Copy Markdown
Member

It looks like the code could be simplified by using pathlib.

mhvk
mhvk reviewed Mar 5, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These seem like good changes, thanks! Do you think it would be possible to test them by expanding utils/tests/test_parser.py? The code looks obviously right to me, but a test would help ensure we don't regress...

As far as the type of change is concerned, I feel this is a feature rather than an API change (or maybe bug fix). I guess it is a bit like performance.

@lpsinger

lpsinger commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

These seem like good changes, thanks! Do you think it would be possible to test them by expanding utils/tests/test_parser.py?

I added a unit test, but it's not yet passing. Even if I remove the byte-compilation it fails, so I think that it is probably my test that is not quite right.

@lpsinger

lpsinger commented Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

Tests ought to pass now. I had to add a t_error rule to the example lexer in the test to get it to pass.

pllim
pllim reviewed Mar 6, 2024
View reviewed changes
Comment thread astropy/utils/tests/test_parsing.py
t.value = int(t.value)
return t

def t_error(t):

@pllim pllim Mar 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this being required an indication of anything more serious?

@lpsinger lpsinger Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Just that PLY lacks a sensible default for t_error if the lexer does not provide one.

mhvk
mhvk approved these changes Mar 6, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, I like the added test - nice & simple. Thanks so much @lpsinger!

@mhvk

mhvk commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Ah, I guess we still have the question of the changelog entry. I'm now a bit less sure than I was before, and think @pllim's suggestion of "other" in the top level is probably most appropriate.

@lpsinger

lpsinger commented Mar 7, 2024

Copy link
Copy Markdown
Contributor Author

Ah, I guess we still have the question of the changelog entry. I'm now a bit less sure than I was before, and think @pllim's suggestion of "other" in the top level is probably most appropriate.

OK, I added a changelog entry.

mhvk
mhvk approved these changes Mar 7, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

mhvk
mhvk reviewed Mar 7, 2024
View reviewed changes
Comment thread docs/changes/16159.other.rst Resolved
@lpsinger
BUG: Fix loading parser tabs on pyc-only installations
9471c8d 
According to the Python module loading
[flow chart](https://peps.python.org/pep-3147/#flow-chart), when
evaluating `import foo` and `foo.py` is not found, Python then
reads `foo.pyc`.

One can take advantage of this fact to strip source files and
leave only Python bytecode files for deployment in
space-constrained execution environments such as AWS Lambda
(see, for example, nasa-gcn/gcn.nasa.gov#2040).

Fix a bug in the wrappers for the lex and yacc wrappers that are
used for parsing Astropy units so that they work on pyc-only
installations.
@mhvk
mhvk enabled auto-merge March 7, 2024 15:13
@mhvk
mhvk merged commit b293ac5 into astropy:main Mar 7, 2024
@lpsinger
lpsinger deleted the lex-pyc branch March 7, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

installation units utils

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@lpsinger @pllim @eerovaher @mhvk