SciTools / cf-units

Units of measure as required by the Climate and Forecast (CF) Metadata Conventions
https://cf-units.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
66 stars 49 forks source link

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

Closed ocefpaf closed 1 month ago

ocefpaf commented 2 years ago

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.

thebaptiste commented 1 year ago

Any news on this issue ?

bjlittle commented 1 year ago

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 :bomb:.

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 :+1:

DManowitz commented 1 year ago

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 commented 1 year ago

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

trexfeathers commented 1 year ago

FYI #368 has now been merged

thebaptiste commented 1 year ago

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

trexfeathers commented 1 year ago

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 commented 12 months ago

@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 commented 11 months ago

@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 commented 6 months ago

@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 commented 1 month ago

@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 :

rcomer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

pelson commented 1 month ago

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 commented 1 month ago

@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.