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

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

Open
jehturner opened this issue Nov 28, 2023 · 15 comments
Open

Comments

@jehturner
Copy link
Member

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
Copy link
Member Author

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 🙂.

@nden
Copy link

nden commented Nov 28, 2023 via email

@jehturner
Copy link
Member Author

jehturner commented Nov 28, 2023

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
Copy link
Member

pllim commented Nov 29, 2023

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
Copy link
Member

olebole commented Nov 29, 2023

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 packages continue to work without changes
  • failures were mainly caused two changes that were deprecated before (update_default_config and angle_utilities)
  • one failure by an API change without deprecation (pyvo; votable.tree.Table --> TableElement)
  • one failure by a change not documented as API change (radio-beam: call convention of units.brighness_temperature).

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
Copy link

nden commented Nov 29, 2023

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

  • newly generated envs with supported versions of Python
  • in stenv which is an "exact" env, where all packages are pinned and tested they work together. stenv is a larger environment - includes JWST and HST related software.

@bsipocz
Copy link
Member

bsipocz commented Nov 30, 2023

@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
Copy link
Member

olebole commented Dec 1, 2023

@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
Copy link
Member

bsipocz commented Dec 1, 2023

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

@pllim
Copy link
Member

pllim commented Dec 1, 2023

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 astropy/astropy#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
Copy link
Member

pllim commented Dec 1, 2023

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
Copy link
Member

bsipocz commented Dec 1, 2023

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? astropy/astropy#14702

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

@pllim
Copy link
Member

pllim commented Dec 1, 2023

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
Copy link
Member

olebole commented Dec 1, 2023

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
Copy link
Member Author

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

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

No branches or pull requests

5 participants