[JOSS Review] Dependency on `astropy`

[Sbozzolo]

pyGWB depends specifically on astropy==5.2. Fixing the dependency version can cause major headaches to package maintainers downstream. Is astropy really needed? As far as I can see, it is used for a couple of physical constants, and for the function io_registry. Maybe you can remove astropy as dependency all together (which would be nice because astropy is not a small package`).

[a-renzini]

astropy is indeed a fundamental dependency of our package (more or less unfortunately); we have been advised to fix the version as indeed the interaction between astropy and other packages (e.g., gwpy) requires a careful version balancing act. It is a bit of a pain but I am willingly in charge of maintaining this dependency and assuring that we always use the most up to date packages, where compatible.

[Sbozzolo]

Could you clarify how is astropy needed as direct dependency?

The main place where I see it used is for i/o in omega_spectra.py, but maybe you could consider using a different format?

Also, the readme says that the package depends on astropy>=5.2, but it should say that requires astropy == 5.2.

The reason I would like to see this pin gone is because it makes developing packages that depend on pyGWB much harder. As you say, there's already a balancing act between other dependencies, and if your package fixes astropy to a particular version, it completely removes any wiggle room.

[a-renzini]

Sure - both bilby and gwpy depend on astropy, so the package needs to be installed for those dependencies to work properly. omega_spectra objects subclass gwpy objects that use astropy i/o, and I can't change that. Plus, in the community we constantly refer to astropy-defined cosmologies which streamlines implementing assumptions consistently across different codes (see the constants module as well).

Thanks for spotting the mistake in the readme - indeed it used to be a weaker dependency but when astropy was upgraded to >5.2 a gwpy functionality broke down, perhaps this has actually been fixed in the meantime so I will also double check this - and make sure the readme is up to date.

I understand your point about making dev. harder, but unfortunately I can't really see how to drop astropy without significantly reducing our functionalities as well as our compatibility with the other packages used. This is a shared burden across the GW coding community, not a unique pygwb problem.

[Sbozzolo]

My point is not with having astropy in the dependency tree, but with pygwb directly depending on a very specific version of astropy for something that seems to be one isolated function.

It shouldn't be your task to find what version of gwpy and astropy work well together. That's up to to the gwpy developers. If the stated version constraints of gwpy do not result in a working package, you should report an issue to their package.

If you can remove direct references to astropy in your package, you'll leave the problem of finding the version that works across all the dependencies to the constraint solver in the package manager, which is the piece of code that is purposefully designed to solve this problem. When you fix the constraint yourself, you are taking all the burden of finding the correct version (which is a rather difficult task when you start to think about multiple versions of packages/Python), and updating that in perpetuity (if you want to have a working package). You are also adding constraints that are not relevant to your package.

As a developer, I had to hold back releases of my packages because upstream dependencies were not careful with their constraints. My impression is that your package is in a good position to not depend directly on astropy. If you do that, you can remove one source of potential problems to potential downstream developers.

omega_spectra objects subclass gwpy objects that use astropy i/o, and I can't change that.

If you are subclassing, can't you use the methods of the parent class for that?

[a-renzini]

It shouldn't be your task to find what version of gwpy and astropy work well together. That's up to to the gwpy developers. If the stated version constraints of gwpy do not result in a working package, you should report an issue to their package.

It shouldn't, but, unfortunately, it has caused issues in the past and the gwpy developers themselves suggested fixing this particular dependency for now to ensure smooth sailing. I was confused as to why this wasn't fixed as a gwpy dependency in the first place (resolving this issue as you point out), I can't speak to why that is.

If you are subclassing, can't you use the methods of the parent class for that?

I had tried this originally, but the reading/writing of the objects was not performing correctly, in particular attributes were not registered properly. I could find no other way than to register the new classes directly.

My impression is that your package is in a good position to not depend directly on astropy.

While dependencies are few, they are still requirements of the package. Other than the registry mentioned above, we also rely on astropy constants and cosmologies. While these may seem like trivial dependencies at first glance, they are in fact instrumental to the package, and the dependency on these is quite possibly set to grow as the pe module grows. Given that astropy is in the dependency tree anyway, I see no reason not to use it, and use it well.

I have verified that our tests still break when admitting astropy>5.2 - this has to do with how gwpy objects are compared to each other in the test suite. this is possibly not something that users would notice, but it is an issue for developers. As our user base is currently developer-dominated, I think fixing this version for now is our safest way forward, as we work with gwpy dev.s to update our test suite. I have not changed it myself as I was under the impression that gwpy devs were considering making changes in their objects which would in fact fix the errors caused by the new astropy version. As I cannot find an issue recording all the work done on this (perhaps issues were also mistakenly closed...), I can open an issue to address the incompatibility with astropy>5.2, either by changing our tests or by monitoring the fixes in gwpy.

[Sbozzolo]

My personal take is that reducing dependencies is always a good thing, especially when they are prone to incompatibilities (such as astropy). All the instances I currently see of astropy seem relatively minor and easily replaceable. Of course, if you expect your reliance to astropy to grow, feel free it keep it as dependency.

In any case, I think you should fix your tests. Pinning the version of astropy to 5.2 feels like a workaround, not something that a production-ready package should do. For example, one of the side effect is every user that installs pygwb in another environment will see their version of astropy downgraded to 5.2 (and astropy 6 is about to be released). Also, while your package doesn't restrict compatibility with Python 3.12, astropy 5.2 won't support it. So, every developer that wants to use your library in a package will have to use astropy 5.2, and won't be able to use Python 3.12.

I believe that this is important to be addressed, but I also recognize my bias on the subject (given all the time I had to invest to work around problems in constraints in upstream dependencies). So maybe the first step might be to least clearly document the problem and identify the way forward?

(The same discussion about unnecessary dependencies goes with seaborn, that is used only to get a color palette (but at least doesn't impose version constraints).)

[a-renzini]

Pinning the version of astropy to 5.2 feels like a workaround, not something that a production-ready package should do.

That's because it was :) it was never meant to be a permanent fix, it was in place to ensure the package worked as its dependencies were updated to be compatible. I admit there is poor documentation about the issue but devs were well aware and I work closely with gwpy and bilby folk to make sure our packages all work together.

I decided to dig a little deeper to see whether the issue had evolved since the version was first pinned, and I was quite pleased to find out that using a fresh verson of gwpy and astropy>5.2 I was able to single out a few minor fixes I could make in pygwb.tests to ensure compatibility. I am now unpinning the astropy version! This is is a new MR where I'll fold in the other edits discussed on the other issues.

[Sbozzolo]

Yay! Thank you very much for all your time and effort spent on this!