astropy / astropy-project

Documents and policies regarding the Astropy Project as a whole.
Creative Commons Attribution 4.0 International
33 stars 44 forks source link

How to handle AstroPy changes breaking affiliated package dependencies? #369

Open jehturner opened 7 months ago

jehturner commented 7 months ago

When testing astropy 6, I noticed that it's possible to get a (partly) broken installation by updating astropy while an older, incompatible gwcs version <0.18 is still installed.

It seems that gwcs versions <0.18 don't work with astropy 6, because they depend on functionality that was later split into the stand-alone asdf-astropy package, which they don't know about. This causes asdf serialization of gwcs objects to fail. While it's the responsibility of gwcs to specify what astropy versions are compatible, it obviously cannot know about all future changes upstream, nor can the dependency constraints of already-published gwcs versions be modified reliably after the fact (because the dependency solver can always choose an older build, before the extra constraint was added, unless old builds are removed from the package repo, which is a Bad Idea for other reasons). Thus, since astropy does not depend on gwcs in turn, there is nothing to prevent the broken combination.

When building the astropy conda package, this particular issue could be avoided by adding "run_constrained: gwcs >=0.18" to the recipe, which would disallow installation alongside incompatible gwcs versions, without actually making gwcs into a dependency. However, I'm not sure whether a similar mechanism exists for pip -- and it's too late now anyway, because 6.0 was already published on PyPI yesterday. So this would only be a partial solution.

This is not the end of the World, because users doing a typical new install should get a working combination of packages, but it would be better if the breakage could not occur when updating astropy or including additional packages that could hold gwcs back.

I'm therefore wondering whether affiliated/dependent packages should perhaps be using constraints such as astropy >=5.0,<6.0 (as conda does for python & numpy), rather than leaving the maximum version wide open, in anticipation of such future breakage? Or maybe some of you have other insights regarding best practices for avoiding dependency hell? I might be making a meal out of a very general problem without a general solution, but it's at least good to be aware of the issue, in order to tell users what to update when they run into it.

(I suppose for DRAGONS, we'll just require gwcs >=0.18 when we publish v3.2 next year and then the issue will mostly go away for us.)

cc: @saimn, @pllim, @mwcraig, @astrofrog, @nden, @olebole

jehturner commented 7 months ago

FWIW, here's a long argument against what I suggested above: https://iscinumpy.dev/post/bound-version-constraints/

Maybe it's not the best approach, but it would be good to have a policy on that :slightly_smiling_face:.

nden commented 7 months ago

Correct me if I'm wrong but I think this is a general issue with Python builds and dependency resolution. I believe there's no general way to solve the issue and as you mentioned there are good reasons not to constrain package versions. The approach we've adopted is

So jwst may constrain gwcs>=0.19.0,<0.20.0

but normally gwcs will not constrain astropy and it is assumed that applications will do it if necessary. This seems to work but of course one has to remember to update dependencies.

I'm curious to hear how others deal with the issue.

jehturner commented 7 months ago

Indeed, though this surprised me a bit, given that astropy & gwcs are closely related packages (maybe an unrealistic expectation). Ah, that's interesting about applications -- but if that's the assumption then I suppose other projects that use the libs need to know about it. Here DRAGONS 3.1 would have had to constrain astropy <6.0 to avoid the issue (does JWST do that?), which brings us back to my dubious suggestion above.

pllim commented 7 months ago

Thanks for opening this issue and sharing your thoughts!

I agree with Nadia in the sense that no amount of pinning would be foolproof. Some sort of integration testing would have helped but the combo that works for you might be meaningless for a different pipeline. If you want to be extra safe, you can do exact (==) pins for your pipeline until you are ready to upgrade, but that would be rather inflexible if your users also use that same environment for, say, arbitrary data analysis.

@nden , didn't your team deploy https://github.com/spacetelescope/stenv ? Does that include pinning? How is that working out for you?

tl;dr -- My personal opinion is that this should be handled downstream, not in astropy core lib.

olebole commented 7 months ago

One additional comment here: Astronomy packages often face the danger to be abandoned by their authors, or at least seem orphaned for quite a while. Very often, they can however still survive because they use only a subset API of their dependencies. Having upper version limits for these dependencies would make them unusable long before they really get buggy with the new version.

I just had the exercise to check the reverse dependencies of Astropy 6.0 in Debian if they still work for the released packages. Result (the list above only contains the failures):

Most of the failures were caused by test code that needed an update (exception: pyvo).

So, I think that having general upper constraints on dependencies is more harmful than it helps. Downstream, one could do that if a DeprecationWarning appears and one is not able to fix that.

What is always helpful: include the tests in the package itself. So, if a user has doubts that a package still works after update, they can always run a python3 -m pytest --pyargs importantpackage.

nden commented 7 months ago

Here DRAGONS 3.1 would have had to constrain astropy <6.0 to avoid the issue (does JWST do that?), which brings us back to my dubious suggestion above.

There was never a reason to pin astropy in jwst, though we don't guarantee the software will work when updating an existing environment. We recommend generating a new environment with new releases of jwst. We guarantee the software will work only in

bsipocz commented 7 months ago

@olebole

one failure by an API change without deprecation (pyvo; votable.tree.Table --> TableElement)

Hmm, that should have been properly deprecated, or was there a mistake made somewhere? Anyway, new pyvo release should fix stuff, we probably get around it next week or the one after that

olebole commented 7 months ago

@bsipocz

Hmm, that should have been properly deprecated, or was there a mistake made somewhere?

In 5.3.4, votable.tree.Table was not marked as deprecated in the source, and also not in CHANGES.rst.

bsipocz commented 7 months ago

It should not be landed in 5.3.4, but 6.0 and https://github.com/astropy/astropy/pull/15372 should have dealt with doing the deprecation rather than renaming it only. Was any of it refactored further before the release?

pllim commented 7 months ago

Re: pyvo

Looking at the log Ole posted on https://github.com/astropy/astropy/wiki/v6.0-RC-Testing#testing-with-coordinatedaffiliatedother-downstream-packages-1

python3-astropy amd64 6.0.0~rc2-2
pyvo 1.4.2-1
...
 29s ___________________________ TestDALResults.test_init ___________________________
 29s 
 29s self = <pyvo.dal.tests.test_query.TestDALResults object at 0x7f64516e1e10>
 29s 
 29s     def test_init(self):
 29s         dalresults = DALResults.from_result_url(
 29s             'http://example.com/query/basic')
 29s     
 29s         assert dalresults.queryurl == 'http://example.com/query/basic'
 29s         assert isinstance(dalresults.votable, VOTableFile)
 29s >       assert isinstance(dalresults.resultstable, VOTable)
 29s E       AssertionError: assert False
 29s E        +  where False = isinstance(<VOTable length=3> ..., VOTable)
 29s E        +    where <VOTable length=3>... = <Table length=3>...
 29s .../pyvo/dal/tests/test_query.py:274: AssertionError

I think it is just a matter of outdated test because this patch was not in the pyvo version under test:

-        assert isinstance(dalresults.resultstable, VOTable)
+        assert isinstance(dalresults.resultstable, TableElement)

In this scenario, pyvo is expecting the deprecated VOTable (which is the Table in astropy.io.votable) but astropy 6.0rc2 was giving back the new TableElement. In the deprecation https://github.com/astropy/astropy/pull/15372 , VOTable/Table now inherits from TableElement as far as astropy is concered, but not the otherway around, so isinstance(TableElement, VOTable) fails.

Therefore, a new pyvo release (where the test is already updated) would solve the pyvo test failure for Ole.

pllim commented 7 months ago

In 5.3.4, votable.tree.Table was not marked as deprecated in the source, and also not in CHANGES.rst.

That is correct. Because it was not deprecated until astropy 6.0.0. You would see that change log for 6.0.0 but not in 5.3.4.

bsipocz commented 7 months ago

assert isinstance(dalresults.resultstable, VOTable)

I can't check on this now, but as I recall this line is misleading as VOTable was an aliased import. I suspect the actual failure is due to the changes in repr? https://github.com/astropy/astropy/pull/14702

Anyway, this should not be an issue for any users whatsoever.

pllim commented 7 months ago

this should not be an issue for any users whatsoeve

I agree on this front. Just a matter of the test needing an update. So, Ole, I think you can even ignore that one test for pyvo for now and your distro would theoretically be fine.

olebole commented 7 months ago

I'll probably just apply the patch VOTable-->TableElement in the test, thank you for the hint. And sorry that this hijacked the discussion topic here.

To bring it back on path: IMO this example supports my argument that forward compatibility is often not a real issue, and causes less problems than blindly applying an upper limit on a dependency.

jehturner commented 7 months ago

And sorry that this hijacked the discussion topic here.

No problem; it's quite a general discussion.

Thanks for everyone's input. I'm just wondering whether we should write down these expectations somewhere (including for users). Eg. I see there's a whole new installation section here, where it might be worth mentioning that dependencies are not guaranteed to work if you update astropy as described without updating them too?:

https://docs.astropy.org/en/stable/install.html