Make antlr optional

[ocefpaf]

Closes #313

[CLAassistant]

All committers have signed the CLA.

[ocefpaf]

I wanted to add a test without antlr-python-runtime but I have no idea how to do that in the complex tox/conda-lock/extra env file setup here. If someone wants to add that note that we would need to call pytest with pytest cf_units/tests to avoid collecting import errors, or use -continue-on-collection-errors to continue.

I can attest that this works. All the functionalities but latex parsing are available and the import error is informative enough, if/when someone tries to use latex.

[pp-mo]

NOTE: update thoughts
Despite the exciting possibilities of #445 and a possible stepping-stone to #446,
that doesn't make a reason to not do this first -- it's still a useful + a simple first step.

[pp-mo]

Status update 2024/9/13 :
this is a good change, but can't just resolve + merge.
Want to incorporate changes to the env ymls, and then re-resolve the lockfiles

[ocefpaf]

it's still a useful + a simple first step.

That would help a lot while we wait for something better.

but can't just resolve + merge.
Want to incorporate changes to the env ymls, and then re-resolve the lockfiles

I confess that the CI setup here grow both in complexity and size that makes me not want to mess with them. Even more so in a case for an optional dependency, which can increase the number of files here. Is that something that is on your radar or as you expecting me, as the PR author, to do it? If the latter, I'd love some guidance before I tackle this.

[trexfeathers]

I confess that the CI setup here grow both in complexity and size that makes me not want to mess with them. Even more so in a case for an optional dependency, which can increase the number of files here. Is that something that is on your radar or as you expecting me, as the PR author, to do it? If the latter, I'd love some guidance before I tackle this.

Hi @ocefpaf, we definitely agree that things have become complex!

Your PR does not need to do anything with lock files

• For testing purposes we make no distinction between core and optional dependencies. This only matters for Pip (i.e. setup.cfg) and Conda (the person updating the feedstock reads cf-units.yml). So no lock-file updates needed.
• Since you raised this, we had a conflicting PR that was also adding Python 3.12, so we've merged that into main and I've updated your branch.

Complexity for developers

The intention is that the complexity is handled by the tox -e py*-lock environments, and that the developer can just commit the results without worrying about them. In future we might even get that to run automatically on any PR that changes cf-units.yml.

We adopted Tox specifically thinking of developers like you, who we don't talk to every day - we want to use familiar tools so we don't need to explain our repo structure to collaborators. What do you think we could we do to make things clearer?

[trexfeathers]

I wanted to add a test without antlr-python-runtime but I have no idea how to do that in the complex tox/conda-lock/extra env file setup here.

Yes, we have (I think) no experience in structuring tests to confirm correct handling of optional dependencies. For instance I can't find any test coverage for this line in Iris.

It doesn't look like PyTest has any built-in conveniences for 'hiding' a dependency for the duration of test(s), but it might be possible using mocking? We're certainly not interested in creating more lock files than we have already. Would love to know your thoughts.

[trexfeathers]

I wanted to add a test without antlr-python-runtime but I have no idea how to do that in the complex tox/conda-lock/extra env file setup here.

Yes, we have (I think) no experience in structuring tests to confirm correct handling of optional dependencies. For instance I can't find any test coverage for this line in Iris.

It doesn't look like PyTest has any built-in conveniences for 'hiding' a dependency for the duration of test(s), but it might be possible using mocking? We're certainly not interested in creating more lock files than we have already. Would love to know your thoughts.

I will do my best to get this over the line either way, even if that means raising an issue to add tests in future.

[trexfeathers]

I will do my best to get this over the line either way, even if that means raising an issue to add tests in future.

#485

[pelson]

The follow-on test infrastructure has quite a cost, when compared to my proposal in #313 (comment):

perhaps just vendoring the runtime (all 150kb of it) is the way to go?

The code is BSD and is pure Python, but it appears to not use relative imports (making it a bit harder to vendor as you have to do a search and replace). The outcome would be no dependency at all, and the tests would validate that the behaviour is as expected.

[pp-mo]

OK I think this is good to merge as-is.
Since we are still installing antlr + running tests, we know this does not break status quo. That's good enough for a start IMHO.

The follow-on #485 is a nice-to have, but not an essential prerequisite.
For sanity we can do a one-off test to check that, right now, cf_units does function without antlr.

[pp-mo]

Since Iris does not currently use the cf_units "tex" function, there should be no need to include this in the Iris dependencies either for testing or runtime (i.e. in the recipe on the iris conda-forge feedstock).
That could change if we move to 'tex()' in iris plots -- we then need to add antlr4-runtime as an Iris test dependency.
Meanwhile, the functionality is still tested here in cf_units, so should not go stale.

[thebaptiste]

Can we hope a new release with antlr optional soon on pypi ?
Thanks

[pp-mo]

Can we hope a new release with antlr optional soon on pypi ? Thanks

I think we're now leaning towards #487 instead.
Presumably that would suit you just as well @thebaptiste ?

But anyway a release before long, though we're not committing to a date yet.

[thebaptiste]

I think we're now leaning towards #487 instead. Presumably that would suit you just as well @thebaptiste ?

Yes, it would suit me, as long as there is no longer a dependency on such an old release (4.7.2) of antlr4-runtime-library.
No dependency at all, vendoring it, is even better !