materialsproject / api

New API client for the Materials Project
https://materialsproject.github.io/api/
Other
105 stars 34 forks source link

Circular dependencies between materialsproject packages #825

Open yurivict opened 1 year ago

yurivict commented 1 year ago

Description

[00:00:13] Error: Dependency loop detected:
These packages depend on each other: py39-pymatgen-2023.6.28 py39-mp-api-0.33.3 py39-pymatgen-analysis-alloys-0.0.6
These packages depend on each other: py39-mp-api-0.33.3 py39-mpcontribs-client-5.4.1 py39-pymatgen-2023.6.28
These packages depend on each other: py39-pymatgen-2023.6.28 py39-mp-api-0.33.3 py39-emmet-core-0.58.0

These messages are printed by the FreeBSD ports framework.

Could you please break these circular dependencies?

janosh commented 1 year ago

I don't think there's an easy way to break the cycle short of dropping mp-api from pymatgen or integrating them into one package.

Can you elaborate how that error is affecting you? I imagine there might be a simpler solution.

yurivict commented 1 year ago

I tried to update the pymetgen port. Other involved dependency ports were created to satisfy dependencies. Then I discovered this circular dependency. FreeBSD packages can't have a circular dependency, and virtually no other Python packages have circular dependencies between them. This is just erroneous to have circular dependencies.

janosh commented 1 year ago

from my understanding, Python doesn't care whether two packages depend on each other as long as the import graphs for any module are non-cyclical. And it's working fine for us in production... so again I think we're going to need more info on what you're trying to do that isn't working to help you out.

yurivict commented 1 year ago

We create FreeBSD ports for all software packages, including pymatgen. Ports and packages can't have circular dependencies.

janosh commented 1 year ago

I'll defer to @shyuep and @munrojm on how to move forward here.

shyuep commented 1 year ago

No comment.

yurivict commented 1 year ago

You can either

  1. Merge them if they really all depend on each other
  2. Remove circular dependencies if possible

Or I can 3. just remove some low-level dependency breaking the circle in an ad-hoc way with a comment.

janosh commented 1 year ago

There's just one mp-api import in pymatgen which is in principle easily removed. But we want to keep the ability to import the new mp_api.MPRester from pymatgen.ext.matproj.

Not sure what you mean by 3.

ml-evs commented 1 year ago

I'd also be in favour of making mp_api and optional dependency in pymatgen (was this not the point of the namespace packages kerfuffle in the first place?), almost every breaking change I encounter across n projects is now related to this dependency circle (see #819 for the most recent one, running into the same deal with pydantic v2 now).

shyuep commented 1 year ago

The so-called namespace package "kerfuffle" was meant to allow other packages that we have no interest in maintaining in core pymatgen to exist in their own domain. And it has led to demonstrable flexibility in the some of the newer add ons. I have always stated that my position is that the mp-api should be part and parcel of pymatgen main package. It is one of the main ways that people get structures and data for use with other pymatgen packages. My position on this is not going to change. So bottom line is whether this dependency circle can be managed. Otherwise, the other two options are (a) move mp-api into the pymatgen repo or (b) mp-api becomes an optional dependency but there will be a second implementation of an MP API interface in pymatgen, like it always has been. After all, the whole "kerfuffle" of an "API" is to allow others to write their own interfaces to that API isn't it?

Or is this too much kerfuffle for people to accept? :-)

ml-evs commented 1 year ago

The so-called namespace package "kerfuffle"

I initially wrote "debacle" but that definitely sounds too serious :wink:

After all, the whole "kerfuffle" of an "API" is to allow others to write their own interfaces to that API isn't it?

Well, the distinction here is that pymatgen still requires mp-api and all of its dependencies for its own interface, so there is no separation of concerns between the pymatgen MP API and the official one. If this is the main use case you want to support for pymatgen then of course that is a valid decision, it is just not very fun to have so much time wasted as a developer of libraries that also depend on pymatgen as a library.

Or is this too much kerfuffle for people to accept? :-)

I'm also not sure I see the value to normal users if they cannot install the package some fraction of the time due to fast-moving/alpha sub-sub-deps! Of course these problems are by no means unique to pymatgen, your just bear the brunt of it due to popularity.

Most likely we will try to fix this downstream by making our own pymatgen dependency optional so at least our packages can be installed, or alternatively, take a much more aggressive approach to pinning all of the sub-dependencies of pymatgen by default and hope that pymatgen itself retains some backwards compatibility with them.

ml-evs commented 1 year ago

I will note that (after hijacking this issue) my problem from #819 is fixed by the upper pin added for mp-api in the most recent versions (so the pain only comes when we have to pin ourselves to older versions), so if you really think this is a solution that can be maintained going forward, then fair enough...

shyuep commented 1 year ago

Well, the distinction here is that pymatgen still requires mp-api and all of its dependencies for its own interface, so there is no separation of concerns between the pymatgen MP API and the official one. If this is the main use case you want to support for pymatgen then of course that is a valid decision, it is just not very fun to have so much time wasted as a developer of libraries that also depend on pymatgen as a library.

Message ID: @.***>

The distinction is that I envision writing an interface that will satisfy 90% of use cases without needing mp-api and its downstream dependencies. People who want all the bells and whistles can still install mp-api.

The one thing you and I agree on is that a lot of time (mine and downstream developers) have been wasted on this. I will be glad to have a solution that silences all these issues once and for all.