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

Circular dependency with mkdocstrings-python-legacy #376

Closed
cpcloud opened this issue Feb 7, 2022 · 21 comments
Closed

Circular dependency with mkdocstrings-python-legacy #376

cpcloud opened this issue Feb 7, 2022 · 21 comments
Labels
packaging Packaging or distribution issues

Comments

@cpcloud
Copy link

cpcloud commented Feb 7, 2022

First off, great work on this project.

We recently converted our API docs at https://docs.ibis-project.org over to mkdocstrings and are extremely happy with the results, so thank you for all the work that you do here.

Describe the bug

We are using poetry2nix to manage dependencies for development environments.

poetry2nix will turn poetry.lock into nix expressions each of which maps to a single Python package.

To do this, poetry2nix will traverse the transitive closure of the set of dependencies specified in poetry.lock. However, it is unable to do so when a project has mkdocstrings==0.18 as a dependency because mkdocstrings 0.18 depends on mkdocstrings-python-legacy which itself depends on mkdocstrings 0.18, resulting in infinite recursion.

To Reproduce
Steps to reproduce the behavior:

  1. Install nix
  2. Download the files in this gist into a directory
  3. Run nix build

Expected behavior

I expect this to work with poetry2nix without infinite recursion.

Information (please complete the following information):

  • OS: NixOS
  • Browser: Brave
  • mkdocstrings version: 0.18.0
@pawamoy
Copy link
Member

pawamoy commented Feb 7, 2022

Hi @cpcloud! Thanks a lot, I'm glad you're enjoying mkdocstrings 😄

Now about the issue at hand. It's true the dependencies are circular, just as you stated: mkdocstrings depends on mkdocstrings-python-legacy which depends on mkdocstrings. But Python resolvers (pip, PDM, Poetry) all seem to be able to resolve this fine. Maybe there's something to do on poetry2nix's side? We're probably their first case of circular dependencies 😄 ?
Let me know if there's anything we can do on our side (but I doubt so, apart from removing the circular dependencies, which I'm not really inclined to do since it seems to be a valid specification).

@pawamoy pawamoy added packaging Packaging or distribution issues and removed unconfirmed This bug was not reproduced yet labels Feb 7, 2022
@pawamoy
Copy link
Member

pawamoy commented Feb 7, 2022

By the way, how does poetry2nix work on PDM projects? PDM generates a pdm.lock file (different name but same format), and we don't even version it. (@adisbladis)
EDIT: ah, you're probably using it on your own pyproject.toml and poetry.lock files.

@pawamoy
Copy link
Member

pawamoy commented Feb 7, 2022

this is not supported by the nixpkgs python infra

Err, that's too bad 😕

nix-community/poetry2nix#1

@cpcloud
Copy link
Author

cpcloud commented Feb 7, 2022

By the way, how does poetry2nix work on PDM projects? PDM generates a pdm.lock file (different name but same format), and we don't even version it. (@adisbladis) EDIT: ah, you're probably using it on your own pyproject.toml and poetry.lock files.

Yeah, pdm is incidental in our case. poetry2nix can work with any of the major Python build tools.

We're probably their first case of circular dependencies?

Possibly, though it's a bit surprising this hasn't come up before outside the various common leaf dependencies like setuptools-scm et al.

apart from removing the circular dependencies

Would you mind elaborating a bit on the rationale for the circularity?

It seems like mkdocstrings should be installable without any dependency (including an optional one) on mkdocstrings-python-legacy, while mkdocstrings-python-legacy definitely requires mkdocstrings.

We have a similar problem in ibis, where the ibis-bigquery project depends on ibis-framework, but ibis itself knows nothing about ibis-bigquery.

Do you see any issue with leaving mkdocstrings-python-legacy out of the mkdocstrings dependency specification altogether while leaving mkdocstrings as a dependency of mkdocstrings-python-legacy?

@hadim
Copy link

hadim commented Feb 7, 2022

Same here, I am creating conda packages for mkdocstring (see conda-forge/staged-recipes#17960 and conda-forge/mkdocstrings-feedstock#17).

Circular dependencies are making the packaging harder (the snake eating itself).

If it's possible to break it, I would favor doing it as I am not sure having circular deps (even your package manager supports it) is a good thing.

@pawamoy
Copy link
Member

pawamoy commented Feb 7, 2022

Would you mind elaborating a bit on the rationale for the circularity?
It seems like mkdocstrings should be installable without any dependency (including an optional one) on mkdocstrings-python-legacy, while mkdocstrings-python-legacy definitely requires mkdocstrings.

Sure!

In fact, removing the dependency on mkdocstrings-python-legacy is planned. It's only here for the transition: previously the legacy Python handler was part of mkdocstrings, and now it lives in this other mkdocstrings-python-legacy project. To give some time to users to adapt, mkdocstrings still hard-depends on it while providing the python-legacy extra (which is a no-op in this context since the legacy handler is always installed).

When imported, the legacy handler warns users that the extra will become mandatory in the next release. This is to make sure nobody gets a bad surprise while upgrading, as I believe the way to deprecate things is, well, to use [deprecation/user] warnings for a short period at least.

It brings us to this: next version of mkdocstrings will only optionally depend on mkdocstrings-python-legacy, which I believe is better supported by other infrastructures tooling, and would not result in infinite recursion?

As to why the circular dependency in the first place: extras are convenient and allow us to depend on mkdocstrings[crystal,python]. Reciprocally, these crystal and python handlers depend on mkdocstrings to make sure both mkdocstrings' and the handlers' versions are compatible.

@hadim
Copy link

hadim commented Feb 7, 2022

@hadim does Conda understands the concept of extras?

Not at the moment. I think it's something the conda folks are thinking about, but it's probably going to take a while.

So usually, it's up to the package maintainer to do what he thinks is best: either include all the extra, none of them or only a few subsets of them.

For the conda packages, extra are not really an issue to me. The circular deps are more critical. But if it's only temporary, then I think it's fine, and I'll upgrade the conda packages once it has been resolved.

@pawamoy
Copy link
Member

pawamoy commented Feb 7, 2022

@hadim Thanks for the explanation, that's reassuring. In your case, I believe including no extras at all would be fine since users can explicitely depend on the handlers themselves.
Same question to you @cpcloud @adisbladis, will poetry2nix/nix choke on the extras, even once the legacy handler is not hard-depended on anymore?

@cpcloud
Copy link
Author

cpcloud commented Feb 21, 2022

@pawamoy Circling back here, I think making mkdocstrings-python-legacy a true optional dependency will suffice!

I tested this by removing the dependency by from poetry.lock, and poetry2nix is able to run.

@bandersen23
Copy link
Contributor

We were able to get the conda-forge package built with all the dependencies needed. I documented the process in conda-forge/mkdocstrings-feedstock#19, but everything is up to date now!

@yajo
Copy link

yajo commented Mar 22, 2022

Have you tried with mach-nix?

@cpcloud
Copy link
Author

cpcloud commented Apr 6, 2022

@pawamoy I looked into this some more, and simply making mkdocstrings-python or mkdocstrings-python-legacy optional will not solve the problem.

Ultimately to work with poetry2nix the dependency graph needs to be free of cycles.

I can work around this problem on my side, but only if both mkdocstrings-python and mkdocstrings-python-legacy are optional dependencies. Otherwise there's a cycle.

Is it possible to keep all the various rendering addons as pure optional dependencies?

@dpaetzel
Copy link

dpaetzel commented Apr 6, 2022

I second that. I'm currently patching the pyproject.toml before building/installing mkdocstrings-python-legacy by doing

sed -i "s/^.*\"mkdocstrings>=.*$//" pyproject.toml

but that can't be the solution in the long term. (Note: I'm using Nix flakes directly, without poetry2nix.)

@yajo
Copy link

yajo commented Apr 8, 2022

If this installs properly with normal python tooling, can't it be a bug in poetry2nix?

@pawamoy
Copy link
Member

pawamoy commented Apr 8, 2022

I kinda agree with @yajo here: extras (optional dependencies) is a pretty standard concept in the Python packaging ecosystem. I'd really like to keep the extras on mkdocstrings 😕 Our circular dependency can't possibly be the only one in the whole ecosystem. It seems poetry2nix special-cases some libraries that cause such circularity, see nix-community/poetry2nix#538, maybe mkdocstrings could be special-cased there as well? Tagging @adisbladis again 👋

I'd appreciate if we could explore a few possibilities, and if everything fails, then I'll remove the extras 🙂
Saving a few lines in pyproject.toml is convenient but probably not worth losing compatibility with other packaging systems 😄

@jpgxs
Copy link

jpgxs commented May 22, 2022

This also affects Bazel projects 👎🏻

ERROR: in py_library rule @python_deps_mkdocstrings//:pkg: cycle in dependency graph:
    //tools/mkdocs:mkdocs (94fc984876740c7bbd36b3ce5fe83bc4802138cbbc4222f011384e5ab773a519)
.-> @python_deps_mkdocstrings//:pkg (94fc984876740c7bbd36b3ce5fe83bc4802138cbbc4222f011384e5ab773a519)
|   @python_deps_mkdocstrings_python_legacy//:pkg (94fc984876740c7bbd36b3ce5fe83bc4802138cbbc4222f011384e5ab773a519)
`-- @python_deps_mkdocstrings//:pkg (94fc984876740c7bbd36b3ce5fe83bc4802138cbbc4222f011384e5ab773a519)

Project looks great by the way - looking forward to using it!

@pawamoy
Copy link
Member

pawamoy commented May 28, 2022

Hi all, just released v0.19, which does not directly depend on mkdocstrings-python-legacy anymore. I would appreciate if you could try again your different tools to see if that still poses a cyclic issue or not.

@jpgxs
Copy link

jpgxs commented May 29, 2022

mkdocstrings is working perfectly with Bazel now 🥳!

Thanks for your great work!

@cpcloud
Copy link
Author

cpcloud commented May 29, 2022

This works with poetry2nix now, with the following caveat: you can't use the extras field to add the python handler like this

mkdocstrings = { version = "...", extras = ["python"] }

Instead, you have to add the handler as a separate dependency:

mkdocstrings = "..."
mkdocstrings-python = "..."

After that, I'm able to get past infinite recursion though griffe is failing to handle __all__ generated by the public library but I'll open a separate issue for that.

@cpcloud cpcloud closed this as completed May 29, 2022
@pawamoy
Copy link
Member

pawamoy commented May 29, 2022

Perfect! Thanks for your feedback @cpcloud @jpgxs 🙂

@cpcloud
Copy link
Author

cpcloud commented May 29, 2022

Here's the (unrelated) griffe issue. Linking here in case anyone hits it later: mkdocstrings/griffe#72.

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

No branches or pull requests

7 participants