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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Mar 7, 2024
Merged

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

merged 1 commit into from Mar 7, 2024

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Mar 5, 2024

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
Copy link

github-actions bot commented Mar 5, 2024

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
Copy link
Member

pllim commented Mar 5, 2024

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
Open
@lpsinger lpsinger mentioned this pull request Mar 5, 2024
Merged
@eerovaher
Copy link
Member

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

mhvk
mhvk reviewed Mar 5, 2024
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

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
Copy link
Contributor Author

lpsinger commented Mar 5, 2024

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
Copy link
Contributor Author

lpsinger commented Mar 6, 2024

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
astropy/utils/tests/test_parsing.py
@@ -35,6 +36,9 @@ def t_NUMBER(t):
t.value = int(t.value)
return t

def t_error(t):
Copy link
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?

Copy link
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
Copy link
Contributor

@mhvk mhvk left a comment

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
Copy link
Contributor

mhvk commented Mar 6, 2024

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
Copy link
Contributor Author

lpsinger commented Mar 7, 2024

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
Copy link
Contributor

@mhvk mhvk left a comment

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
docs/changes/16159.other.rst Outdated Show resolved Hide 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
27 checks passed
@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.

None yet
4 participants
@lpsinger @pllim @eerovaher @mhvk