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

Upgrade Antlr to 4.11 and vendor the runtime #1114

Merged
merged 51 commits into from
Feb 15, 2024
Merged

Conversation

jlopezpena
Copy link
Contributor

Bumping antlr binary and antlr4-python3-runtime in order to unlock doing the same thing in hydra, following this discussion

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

Thanks @jlopezpena!
This looks good to me.
I have confirmed that the sha256 of the antlr .jar file in this PR is the same as the sha256 of the .jar file available at github.com/antlr/website-antlr4.

@Jasha10 Jasha10 linked an issue Aug 9, 2023 that may be closed by this pull request
@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 9, 2023

(CI lint failures are unrelated to this PR)

@omry
Copy link
Owner

omry commented Aug 14, 2023

@Jasha10, this is a breaking change (that would also require a parallel fix to Hydra.

  1. News fragment.
  2. I think it's really best if the effort is focused on fixing the root issue instead of bumping the version like this. In the mean time users with specific antlr version needs can build their own version of OmegaConf.

@astrojuanlu
Copy link

astrojuanlu commented Aug 14, 2023

In the mean time users with specific antlr version needs can build their own version of OmegaConf.

How should users (like myself) do that, in a way that it works with various dependencies declaring omegaconf in their metadata?

(To keep the comment on-topic: from the peanut gallery, looks like this is effort well spent on a short term solution while folks address the root cause)

@jlopezpena
Copy link
Contributor Author

@omry what is the root issue that needs to be fixed, and is there any way I can help with that?

I submitted the corresponding PR to hydra (in draft state as it requires a new version of omegaconf).

@jlopezpena
Copy link
Contributor Author

jlopezpena commented Aug 14, 2023

How should users (like myself) do that, in a way that it works with various dependencies declaring omegaconf in their metadata?

@astrojuanlu You can do that by having your own dependency repository (kind of a mirrored version of PyPi) where you make specific builds of packages catered to your needs. Then when you declare dependencies you specify your mirror as the primary place to find dependencies and PyPi as a fallback, or explicitly declare dependencies to be pulled from your mirror.

Eg with poetry you can add something like this in your pyproject.toml:

[[tool.poetry.source]]
name = "PyPI"
priority = "default"

[[tool.poetry.source]]
name = "your_source"
priority = "supplemental"
url = URL_TO_YOUR_MIRROR

and when you declare a dependency instead of doing
omegaconf = 2.3.0
you do something like
omegaconf = {version = "2.3.0", source = "your_source"}

The problem with this approach is if you are working on a library that needs to be used by other people in their project, because then you are forcing THEM to also do the same thing (or you to vendor the modified package source for omegaconf and include it in the code you distribute)

@astrojuanlu
Copy link

The problem with this approach is if you are working on a library that needs to be used by other people in their project, because then you are forcing THEM to also do the same thing

Yeah that's what I had in mind, ideally we should be able to do this in a way that transitive dependencies don't have to cooperate. Otherwise I don't see how users can cope with the current situation effectively.

@shchur
Copy link

shchur commented Aug 18, 2023

Hello, just wanted to clarify what the status of this PR is. Does the comment by @omry mean that this PR will be blocked and we will need to find another way to make omegaconf compatible with different antlr4-python3-runtime versions (without upgrading to antlr4-python3-runtime==4.11.*)?

If this is the case, I would be happy to help with finding the root cause and working on a PR to address it.

@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 19, 2023

I think it would be good to at least discuss @omry's points before merging.

@Jasha10, this is a breaking change

Yes, we are working on that: facebookresearch/hydra#2733

  1. News fragment.

Done! (commit bc4949b)

  1. I think it's really best if the effort is focused on fixing the root issue instead of bumping the version like this.

Any ideas for fixing the root issue? The antlr version is tightly coupled to the minor version of antlr4-python3-runtime.

In the mean time users with specific antlr version needs can build their own version of OmegaConf.

Here is a recipe for users who need to build their own version of OmegaConf+Hydra:
facebookresearch/hydra#2491 (comment)

My opinion is that we should go forward with this PR.

@odelalleau
Copy link
Collaborator

I haven't looked at this in details, but would it be possible by any chance to make Hydra / OmegaConf compatible with multiple versions of antlr? (compiling the grammar during pip install instead of shipping the pre-compiled files)

@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 20, 2023

would it be possible by any chance to make Hydra / OmegaConf compatible with multiple versions of antlr? (compiling the grammar during pip install instead of shipping the pre-compiled files)

The current workflow for compiling the grammar invokes the following command:

java -jar build_helpers/bin/antlr-4.9.3-complete.jar -Dlanguage=Python3 -o omegaconf/grammar/gen -Xexact-output-dir -visitor omegaconf/grammar/grammar

This requires the antlr .jar to be present when the grammar is compiled.

If we compile the grammar during pip install then I think we would have to either (1) ship multiple antlr .jar files and select one of them at install time, or (2) expose an option for the user to provide their own antlr dependency.

Option (2) has been suggested before (here). I'm not sure specifically how we would expose such an option at pip install time -- maybe via an extras option?

@natephysics
Copy link

It just so happens that I've run across a dependency problem involving this exact issue as well. Seems like a tricky issue to solve.

@omry
Copy link
Owner

omry commented Aug 30, 2023

I think the best solution would be to "vendor" OmegaConf with a specific version of antlr, and have Hydra depend on it.
That would require some rewriting some imports in the generated code and would essentially eliminate the external dependency on antlr.

I think I outlined it at some point as one of the options for a permanent fix for this issue.

@jlopezpena
Copy link
Contributor Author

Could an option be to replace the anttrl4 java binary by antlr-tools?

@omry
Copy link
Owner

omry commented Sep 2, 2023

Could an option be to replace the anttrl4 java binary by antlr-tools?

I don't see how that would do anything.
The antlr java binary is just a matter of convenience during the build of OmegaConf.
The problem is that the antlr Python runtime only works with generated code that is of the exact same version used to generate the parser.

@jlopezpena
Copy link
Contributor Author

I see, so the binary dependency is not runtime, but build-time only. I can think of two potential solutions to the bigger issue then.

One would be to build the grammars at install time. The downside of that is that the antlr binary would need to be downloaded at install time (probably through antlr-tools to avoid needing a system wide install of java). This would add size and time to install, as well as require an active internet collection.

The second approach would be to build multiple versions of the grammars at build time (through multiple versions of the antlr binary), and select the grammar at runtime based on the installed version of antlr4-python3-runtime . This would effectively decouple the build dependency from run time, at the cost of having additional grammar files distributed with the wheel and needing the (slightly) more complex logic for dispatching the correct version.

I believe the second option would be nicer from the user's point of view, with the tradeoff of having a more complex build process (which might or might not be acceptable for omegacong/hydra maintainers)

@odelalleau
Copy link
Collaborator

I think @omry's suggestion of shipping a specific version of antlr4-python3-runtime within OmegaConf may be the most robust approach.

@omry
Copy link
Owner

omry commented Sep 5, 2023

I see, so the binary dependency is not runtime, but build-time only.

Not quite. The antlr runtime is checking that it's working with code generated by a generator from the exact same version.
This is a problematic design choice, as in reality antlr is very stable and they could be much laxer with this check - but I digress.

Other low-level libraries, like pip itself, solves the problem of dependency conflicts by optionally including their dependencies in under a different module. They call this vendoring (pip page about it).
The idea is that by including your own version of the dependency, you isolate your dependencies from the dependencies of userland apps and eliminating these kinds of issues.

The complication is that Hydra should probably follow whatever OmegaConf is doing here.

@jlopezpena
Copy link
Contributor Author

jlopezpena commented Sep 5, 2023

I understand the benefit of vendoring, the downside of that is that by vendoring a third party pakage you are left with the burden of incorporating security updates and whatnot yourself - which might or might not be a big ordeal depending on how often there are changes that need to be incorporated, and how complex updating the vendoring is (which might be as simple as running a git submodule update or a much more complex one). That's why in my own projects I tend to shy away from vendoring and normally opt for something similar to the second option I mentioned about (this is also the approach taken in many conda recipes that require binary linking against a specific version of numpy, resulting in a different "precompiled" binary for each python/numpy/architecture combination).

As mentioned, if there is anything I can do to help solve the issue let me know, the final decision on what the right solution is of couse falls onto the core developers, as they are the ones that will be left with the maintenance burden.

@astrojuanlu
Copy link

Is there a final decision on what the way forward should be? We are discovering more incompatibilities with other dependencies.

@odelalleau
Copy link
Collaborator

Is there a final decision on what the way forward should be? We are discovering more incompatibilities with other dependencies.

I think shipping our own version will be easier overall. We just need someone to actually implement it :)

@astrojuanlu
Copy link

I think shipping our own version will be easier overall.

I guess you mean @omry's vendoring idea from #1114 (comment)

We just need someone to actually implement it :)

👍🏽 Thank you!

@jlopezpena
Copy link
Contributor Author

I can reproduce the problem. The files in vendor/antlr4 are getting copied over, but folders inside that are not. Will try to work on a fix later today or tomorrow. Thanks for flagging it up @mawildoer !

@jlopezpena
Copy link
Contributor Author

@mawildoer the non-editable installs should work now, could you give it a check?

@Jasha10, @omry it looks like CI is only testing editable installs, perhaps a non-editable scenario should be added as well (in a separate PR)

@mawildoer
Copy link

I don't know if it's related but note that direct setup.py invokation on the CLI is deprecated, see https://packaging.python.org/en/latest/discussions/setup-py-deprecated/#what-commands-should-be-used-instead

TIL; those are the old-school alias for the -e we know and love! I've only ever used "speciality" setup.py commands once before, for something more analogous to the antlr case there (eg. doing something special). I just saw them this time in the setup.py and figured it was worth giving a whirl, since other things weren't working yet. Thanks for the comment!

@mawildoer the non-editable installs should work now, could you give it a check?

Yessir! Tested both pip install -e . and pip install .. Both working 🎉 w/ full IDE/pyright/pylint etc... finding them too.

Thanks for the quick response! I'll have to package and distribute your branch if we can't get this back to mainline soon 😅

Thanks for all the work!

@Jasha10
Copy link
Collaborator

Jasha10 commented Nov 29, 2023

@jlopezpena I've just uploaded omegaconf dev release 2.4.0.dev1 to pypi based on this PR branch. This should be useful so there's something to pin against when working on the Hydra PR.
https://pypi.org/project/omegaconf/2.4.0.dev1/

pip install omegaconf==2.4.0.dev1

@jlopezpena
Copy link
Contributor Author

@jlopezpena I've just uploaded omegaconf dev release 2.4.0.dev1 to pypi based on this PR branch. This should be useful so there's something to pin against when working on the Hydra PR.

@Jasha10 excellent, thank you very much! I made a commit to the hydra PR including the regex import fixing for the generated parsers, will change it to use the 2.4.0.dev1 version of omegaconf and start testing on Monday.

@jlopezpena
Copy link
Contributor Author

jlopezpena commented Dec 2, 2023

It would be useful if someone can try a non-editable install on Windows -- I don't have a windows machine, and am a bit concerned that the "walk-path-and-replace" strategy for finding the submodules might not work due to some path formatting issues (I changed the replacement to use os.path.sep instead of '/', but if os.path.walk prints the path in absolute form instead as relative as in POSIX, the replacement might not work as intended)

@shchur
Copy link

shchur commented Jan 17, 2024

Hi @Jasha10, what are the remaining TODOs before this PR can be merged?

Currently, the antlr version incompatibility is major blocker for our project (because of the clash between Omegaconf & https://github.com/Nixtla/statsforecast), so I would be happy to help with any of the remaining TODOs, if possible.

@Jasha10
Copy link
Collaborator

Jasha10 commented Jan 18, 2024

In my opinion this PR is in good shape.
I hoped to merge this at about the same time as it's sister PR facebookresearch/hydra#2733 where unfortunately we have a backlog of CI issues that I believe are mostly unrelated to the PR itself (rather just technical debt on the Hydra project).

@shchur
Copy link

shchur commented Jan 18, 2024

@Jasha10, thank you for the quick response! Are there some open issues related to the CI failures on the hydra side that I could take a stab at?

@odelalleau
Copy link
Collaborator

odelalleau commented Jan 18, 2024

@Jasha10, thank you for the quick response! Are there some open issues related to the CI failures on the hydra side that I could take a stab at?

There are no open issues but you can basically look at recent open PRs on Hydra to check what's failing.

@Jasha10
Copy link
Collaborator

Jasha10 commented Jan 19, 2024

@shchur I have made a list:
facebookresearch/hydra#2841

@Jasha10 Jasha10 merged commit 52a2e2b into omry:master Feb 15, 2024
1 of 5 checks passed
@Jasha10
Copy link
Collaborator

Jasha10 commented Feb 15, 2024

Thank you @jlopezpena for taking on this vendoring project.
The PR is merged and omegaconf dev release 2.4.0.dev2 is now available on pypi.
https://pypi.org/project/omegaconf/2.4.0.dev2/

pip install omegaconf==2.4.0.dev2

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

Successfully merging this pull request may close these issues.

Request support for anltr 4.11.1
8 participants