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

Vendor the antlr4 runtime library #487

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Conversation

pelson
Copy link
Member

@pelson pelson commented Sep 27, 2024

  • Vendors the antlr4 runtime. The copyright and license information (BSD) is included in the header of each of the vendored files (because that is how the code is shipped by antlr4-python3-runtime)
  • Drops the latex extra - it is no longer necessary/used to have the antlr4 runtime library installed (since we vendored it)
  • Re-formats all of the generated code - this makes life easier to dig into the code, and also means that there is less likely to be diffs when upstream code becomes more standardised wrt. formatting
  • Makes generated code not shown by default in a PR.

pyproject.toml Show resolved Hide resolved
@pelson
Copy link
Member Author

pelson commented Sep 27, 2024

Sadly, the simplest vendoring approach of using absolute import names fails...

/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/importlib/__init__.py:126: in import_module
      return _bootstrap._gcd_import(name[level:], package, level)
  ../venv-test-x86_64/lib/python3.10/site-packages/cf_units/tests/integration/parse/test_parse.py:11: in <module>
      from cf_units._udunits2_parser import normalize
  ../venv-test-x86_64/lib/python3.10/site-packages/cf_units/_udunits2_parser/__init__.py:8: in <module>
      from cf_units._udunits2_parser.parser._antlr4_runtime import (
  ../venv-test-x86_64/lib/python3.10/site-packages/cf_units/_udunits2_parser/parser/_antlr4_runtime/__init__.py:8: in <module>
      from cf_units._udunits2_parser.parser._antlr4_runtime.atn.ParserATNSimulator import (
  ../venv-test-x86_64/lib/python3.10/site-packages/cf_units/_udunits2_parser/parser/_antlr4_runtime/atn/ParserATNSimulator.py:236: in <module>
      from cf_units._udunits2_parser.parser._antlr4_runtime import DFA
  E   ImportError: cannot import name 'DFA' from partially initialized module 'cf_units._udunits2_parser.parser._antlr4_runtime' (most likely due to a circular import)

Looks like I will have to do a bit more work to do relative imports.

@pelson pelson force-pushed the feature/compile-update branch 3 times, most recently from c1d360b to 350bca5 Compare September 29, 2024 06:04
@pelson
Copy link
Member Author

pelson commented Oct 1, 2024

The codacy test is failing. I don't know how to configure the tool, and can't run it locally. I don't see any docs on this topic other than https://docs.codacy.com/repositories-configure/codacy-configuration-file/. I also don't have an option to configure the project in the codacy website (screenshot:
Screenshot from 2024-10-01 06-36-24
).

Would you have any suggestions on how best to proceed @trexfeathers?

@pelson pelson force-pushed the feature/compile-update branch 2 times, most recently from 95de3af to b0b38bc Compare October 1, 2024 07:28
@trexfeathers
Copy link
Collaborator

Would you have any suggestions on how best to proceed @trexfeathers?

In short: no. I'm afraid I don't know anything about Codacy, other than the fact that it occasionally produces confusing objections. I hope I'm not picking names out of a hat: @bjlittle / @lbdreyer can either of you speak to why cf-units uses Codacy where most of our other repos use Codecov?

@bjlittle
Copy link
Member

bjlittle commented Oct 2, 2024

We dabbled with Codacy adoption, but for me it's out of favour now.

Personally, I think we should drop it's usage as codecov and ruff easily replace it along with repo-review 👍

@lbdreyer
Copy link
Member

lbdreyer commented Oct 2, 2024

In short: no. I'm afraid I don't know anything about Codacy, other than the fact that it occasionally produces confusing objections. I hope I'm not picking names out of a hat: @bjlittle / @lbdreyer can either of you speak to why cf-units uses Codacy where most of our other repos use Codecov?

I'm afraid I don't know. I remember briefly talking about codacy a couple of years ago. We thought it may be nice as it had some extra functionality over other coding standards/coverage/checking tools. I believe I gave it access to the SciTools org, but I don't remember enabling it for cf-units. Perhaps that was someone else?
I remember deciding codacy wasn't suitable, but I can't remember why now!

@trexfeathers
Copy link
Collaborator

We dabbled with Codacy adoption, but for me it's out of favour now.

Personally, I think we should drop it's usage as codecov and ruff easily replace it along with repo-review 👍

OK sounds good. @pp-mo as of Monday you are the 'developer in charge' here; are you happy with us switching to Codecov? I'm fairly confident I can see how to make it happen.

@bjlittle
Copy link
Member

bjlittle commented Oct 2, 2024

codacy has now been unconfigured from cf-units (and scitools, as cf-units was the only repo using it)

@trexfeathers
Copy link
Collaborator

codacy has now been unconfigured from cf-units (and scitools, as cf-units was the only repo using it)

#497

@pelson
Copy link
Member Author

pelson commented Oct 3, 2024

Requesting a review of this before a release is considered. cc @pp-mo as I think #423 took us in the wrong direction.

@pelson
Copy link
Member Author

pelson commented Oct 3, 2024

Also: big thanks for updating/removing the codacy configuration - it was a bit uncomfortable that I had no idea what was going on, how to configure it, nor how to run it locally. Much appreciated that it got straightened out! 👍

@rcomer
Copy link
Member

rcomer commented Oct 3, 2024

I don't think I'm qualified to review this, but I think it would be good to update the README so future devs know how to look after this. In particular, the section I added last year about updating antlr would no longer be correct I think.

@pp-mo pp-mo mentioned this pull request Oct 3, 2024
@pelson
Copy link
Member Author

pelson commented Oct 4, 2024

Thanks for pointing it out. I've rebased and added the comment to the README.

@pp-mo
Copy link
Member

pp-mo commented Oct 16, 2024

Despite the apparent silence, I'm keen to include this in a v3.3 release.

We now think will be within 1-2 weeks to provide full working with numpy2 for Iris 3.11
-- that is by 2024-10-28

I did have some suggested addition to the readme, but otherwise about ready to merge this.
Sorry for the delay, I'll post that soon.

I'm assuming that the work in https://github.com/pelson/pyudunits2/ has not affected your enthusiasm for this approach @pelson ?

Copy link
Member Author

@pelson pelson left a comment

Choose a reason for hiding this comment

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

I'm assuming that the work in https://github.com/pelson/pyudunits2/ has not affected your enthusiasm for this approach @pelson ?

No, I still think this is the right thing to do here (I lifted it verbatim in pyudunits2).

I wrote a few comments for which I forgot to hit "submit review", so include them when commenting now (they may or may not be useful...)

cf_units/_udunits2_parser/compile.py Show resolved Hide resolved
cf_units/_udunits2_parser/compile.py Show resolved Hide resolved
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

All ok, but I just wanted the description of how we do/don't need antlr4 even clearer in the README.
Does this sound OK @pelson ?

Wanting to merge this so we can get it in 3.3, of course

cf_units/_udunits2_parser/README.md Outdated Show resolved Hide resolved
cf_units/_udunits2_parser/README.md Outdated Show resolved Hide resolved
pelson and others added 2 commits October 30, 2024 19:23
Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 53.82018% with 2599 lines in your changes missing coverage. Please review.

Project coverage is 58.41%. Comparing base (792ba1e) to head (04bdd7a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...2_parser/_antlr4_runtime/atn/ParserATNSimulator.py 59.87% 182 Missing and 66 partials ⚠️
...er/_antlr4_runtime/tree/ParseTreePatternMatcher.py 15.76% 171 Missing ⚠️
...f_units/_udunits2_parser/_antlr4_runtime/Parser.py 48.47% 133 Missing and 19 partials ⚠️
...its2_parser/_antlr4_runtime/atn/SemanticContext.py 28.87% 129 Missing and 4 partials ⚠️
...its2_parser/_antlr4_runtime/TokenStreamRewriter.py 25.71% 130 Missing ⚠️
...its2_parser/_antlr4_runtime/error/ErrorStrategy.py 49.75% 91 Missing and 13 partials ⚠️
...units2_parser/_antlr4_runtime/PredictionContext.py 69.64% 76 Missing and 26 partials ⚠️
...ts/_udunits2_parser/_antlr4_runtime/xpath/XPath.py 28.77% 99 Missing ⚠️
...its2_parser/_antlr4_runtime/BufferedTokenStream.py 46.51% 80 Missing and 12 partials ⚠️
...its2_parser/_antlr4_runtime/atn/ATNDeserializer.py 70.23% 70 Missing and 19 partials ⚠️
... and 40 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #487       +/-   ##
===========================================
- Coverage   90.50%   58.41%   -32.10%     
===========================================
  Files           6       62       +56     
  Lines         811     6435     +5624     
  Branches       93     1150     +1057     
===========================================
+ Hits          734     3759     +3025     
- Misses         64     2385     +2321     
- Partials       13      291      +278     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pp-mo
Copy link
Member

pp-mo commented Oct 30, 2024

The coverage dropout is inevitable -- no real problem.

@pp-mo pp-mo merged commit 04a4350 into SciTools:main Oct 30, 2024
14 of 15 checks passed
@pelson pelson deleted the feature/compile-update branch October 30, 2024 20:41
@pelson
Copy link
Member Author

pelson commented Oct 30, 2024

Thanks for following-up!

The coverage dropout is inevitable -- no real problem.

There would be no harm in configuring coverage to ignore the committed files though. Might be worth it to get a more accurate reflection of the rest of the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants