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

Can we make antlr4-python3-runtime==4.7.2 dependency optional? #313

Closed
ocefpaf opened this issue Nov 1, 2022 · 16 comments · Fixed by #423
Closed

Can we make antlr4-python3-runtime==4.7.2 dependency optional? #313

ocefpaf opened this issue Nov 1, 2022 · 16 comments · Fixed by #423

Comments

@ocefpaf
Copy link
Member

ocefpaf commented Nov 1, 2022

It is getting quite hard to package cf_units with such an old dependency. My guess is that this could be make optional if a user doesn't want the latex representation of the units, right?

Another alternative would be to update that dependency but I'm not sure how hard that would be. I tried a naive update and got some quite odd and cryptic errors.

@ocefpaf ocefpaf added the New: Issue Highlight a new community raised "generic" issue label Nov 1, 2022
@thebaptiste
Copy link

Any news on this issue ?

@bjlittle
Copy link
Member

bjlittle commented May 25, 2023

Hey @thebaptiste,

Thanks for the nudge. It's great to know that you're keen for this to happen. I am too.

I'm keen to investigate and move this along, as this dependency pin is a ticking time 💣.

It's certainly not fallen off my radar, but to be honest ATM I'm totally maxed-out on other project work. But I'll try to prioritize and get to it ASAP 👍

@bjlittle bjlittle added Type: Tech Debt and removed New: Issue Highlight a new community raised "generic" issue labels May 25, 2023
@DManowitz
Copy link

DManowitz commented Jun 22, 2023

Or would it at least be possible to update to newer antlr dependency? Looking on conda-forge, many newer packages seem to want antlr4-python3-runtime 4.11.1. On a slightly separate note, if you are going to make a build without antlr as a dependency or with a newer antlr runtime, could you also make it available for py38 for at least 1 version?

@rcomer
Copy link
Member

rcomer commented Jun 27, 2023

I had a go at updating the version in #368. It seems to work but I have not understood much...

@trexfeathers
Copy link
Collaborator

FYI #368 has now been merged

@thebaptiste
Copy link

thebaptiste commented Sep 13, 2023

So we hope for a new release on pypi soon !
These developers are never happy and always want everything right away... Sorry !

@trexfeathers
Copy link
Collaborator

So we hope for a new release on pypi soon ! These developers, are never happy and always want everything right away... Sorry !

To re-assure you: this is on the radar soon.

@pp-mo
Copy link
Member

pp-mo commented Nov 15, 2023

@SciTools/peloton it seems that #368 has resolved this problem, but only "for now".
That will presumably be OK for the next release.
But in the nature of how antlr works, we think we will always have to pin the version.

@ocefpaf does this satisfy, or would you argue for removing this altogether ?

@monego
Copy link

monego commented Dec 2, 2023

@SciTools/peloton it seems that #368 has resolved this problem, but only "for now". That will presumably be OK for the next release. But in the nature of how antlr works, we think we will always have to pin the version.

@ocefpaf does this satisfy, or would you argue for removing this altogether ?

Pinning would be an issue for Linux packages as most distributions install them in a global space (/usr) and cannot have multiple versions of the same package installed simultaneously. So they ship a single version and may patch upstream as needed.

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 23, 2024

@ocefpaf does this satisfy, or would you argue for removing this altogether ?

Definitely not removing it but I wonder if we can make it optional. I'm not sure if that is possible. I'll give it a go...

@pp-mo
Copy link
Member

pp-mo commented Sep 12, 2024

@ocefpaf does this satisfy, or would you argue for removing this altogether ?

Definitely not removing it but I wonder if we can make it optional. I'm not sure if that is possible. I'll give it a go...

FWIW just looking into this, I realised that Iris itself does not actually use this functionality (the tex function) anywhere.
Which surprised me. But it does raise the question how important this is, going forward.

Some other ideas emerging :

  • I think it's fair to say that the maintenance difficulty this introduces is really about antlr-python.
    ANTLR4 itself is a well-used and well-maintained tool, but it is in Java.
    It's pretty much that same as with udunits2 : The code itself is easy to maintain + rebuild, it is Cython and the python wrapper that require tricky updates to maintain support across different pythons, architectures, or OS-s
  • the idea of writing a python-native parser is another possible approach.
    It doesn't seem worth the effort just for this, but it might be a useful first step to producing a pure-python cf_units

@rcomer
Copy link
Member

rcomer commented Sep 12, 2024

But it does raise the question how important this is, going forward.

I wonder how many users even know it exists, as it does not seem to be mentioned in the docs.

@trexfeathers
Copy link
Collaborator

But it does raise the question how important this is, going forward.

I wonder how many users even know it exists, as it does not seem to be mentioned in the docs.

That's my bad - @trexfeathers' first attempt at Sphinx & Readthedocs, several years ago 😳

@rcomer
Copy link
Member

rcomer commented Sep 13, 2024

@trexfeathers I don't see it in the legacy docs either, so I'm not convinced it's your fault.

@pelson
Copy link
Member

pelson commented Sep 13, 2024

The original motivation is written up in https://pelson.github.io/2019/cf_units_tex/.

The development was about providing more flexibility when it comes to unit handling, and there is desirable functionality that udunits is not able to (and seemingly will never) support (e.g. maintaining mixing ratio units).

My guess is that in reality, there is very little (no) usage of it because there was no follow-up to actually use the underlying infrastructure. An obvious step would be to update iris' quick plotting routines to render the units well by default, for example.

From a bigger picture perspective, I saw the ability to handle udunits parsing properly as an important step towards potentially defining a CF-conventions specification for units without deferring to an opaque implementation (udunits) - CF-conventions would then instead be able to say "udunits is the reference implementation of this well defined specification". Clearly such a grand vision requires a lot off effort to follow-up, and I stopped working in the space not long after the parsing feature was introduced.


Background aside, as I commented in #445 - the ANTLR runtime is a lightweight pure-python dependency. There is indeed a complexity in matching the version of the runtime from the parser-generator - I don't know how flexible ANTLR is wrt. parsers generated with one version working with a different runtime. However, I have full confidence in the testing of the parser (it was a life-saver on many occasions when developing the grammar), and would suggest that dependabot updating the version is totally fine if the tests pass. As for how this all fits into conda-forge - perhaps just vendoring the runtime (all 150kb of it) is the way to go?

@pp-mo pp-mo assigned pp-mo and trexfeathers and unassigned bjlittle Sep 13, 2024
@pp-mo
Copy link
Member

pp-mo commented Sep 25, 2024

@pelson An obvious step would be to update iris' quick plotting routines to render the units well by default,

👍

But I'm not so sure about

defining a CF-conventions specification for units

From my reading of CF discussions, I haven't seen any desire to do this : CF are so far content that udunits2 is good enough and, in the absence of any serious need to extend it, will just continue to defer to UDUNITS to manage the spec + reference implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants