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

Remove legacy extension api #1464

Merged
merged 13 commits into from
May 8, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Mar 6, 2023

This PR removes the public api for the deprecated legacy extensions.

Internally, ASDF still uses this api for building and registering the BuiltinExtension so removal of the legacy extension methods and functions is not possible at this point.

Some objects were made private to prevent their inclusion in the documentation (AsdfType and AsdfExtension).

The asdf_extension entry point is preserved but modified to only load the BuiltinExtension. This modification means that many types previously converted by astropy.io.misc.asdf are now converted by asdf-astropy. This required updates to the test_version_map_support test to match the supported tags in asdf-astropy (and to make this test aware of the new extensions).

@braingram braingram added the development No backport required label Mar 6, 2023
@github-actions github-actions bot added this to the 3.0.0 milestone Mar 6, 2023
@braingram braingram force-pushed the remove_legacy_extension_api branch 2 times, most recently from 0103147 to 0f1f1d7 Compare March 7, 2023 16:50
@braingram braingram marked this pull request as ready for review March 7, 2023 18:42
@braingram braingram requested a review from a team as a code owner March 7, 2023 18:42
@braingram braingram force-pushed the remove_legacy_extension_api branch from 8529c69 to 0ddbd1e Compare April 5, 2023 14:21
@braingram
Copy link
Contributor Author

braingram commented Apr 5, 2023

weldx downstream job failure for this PR looks like it should be fixed by weldx PR: BAMWelDX/weldx#863

@CagtayFabry
Copy link
Contributor

weldx downstream job failure for this PR looks like it should be fixed by weldx PR: BAMWelDX/weldx#863

Yes we will release a new version soon based on the new extension API which should fix the downstream job

@CagtayFabry
Copy link
Contributor

feel free to rerun the downstream welx test again now with weldx 0.6.5 @braingram

@braingram braingram force-pushed the remove_legacy_extension_api branch 2 times, most recently from a2d1aaf to 95ee7c0 Compare April 24, 2023 19:44
@braingram braingram force-pushed the remove_legacy_extension_api branch 2 times, most recently from 48ab7a7 to d58a895 Compare May 2, 2023 18:58
@braingram
Copy link
Contributor Author

WeldX downstream failure related to new version of pint: BAMWelDX/weldx#872

Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one silly thing of little consequence, LGTM either way.

image

Check out that ratio!

asdf/_types.py Outdated Show resolved Hide resolved
@@ -254,23 +252,6 @@ def extension_manager(self):
self._extension_manager = get_cached_extension_manager(self._user_extensions + self._plugin_extensions)
return self._extension_manager

@property
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Begone!

@braingram braingram force-pushed the remove_legacy_extension_api branch from 5437d52 to 19dc360 Compare May 8, 2023 13:19
@braingram
Copy link
Contributor Author

FWIW: devdeps are failing on python 3.8 because gwcs has dropped python 3.8: spacetelescope/gwcs#451

@braingram braingram merged commit 587542e into asdf-format:main May 8, 2023
@braingram braingram deleted the remove_legacy_extension_api branch May 8, 2023 13:52
@pllim
Copy link
Contributor

pllim commented May 9, 2023

This might have broken 2 of the astropy release branches: v5.2.x and v5.3.x

astropy v6.0.dev seems compatible but that is not going to be released until the end of 2023. Any chances this can be reverted and then wait 6 months? It is the v5.3.x that I worry about.

Example logs:

cc @WilliamJamieson

@pllim
Copy link
Contributor

pllim commented May 9, 2023

p.s. I opened astropy/astropy#14795

@braingram
Copy link
Contributor Author

astropy v6.0.dev seems compatible but that is not going to be released until the end of 2023. Any chances this can be reverted and then wait 6 months? It is the v5.3.x that I worry about.

I would prefer to find a solution that does not revert and delay these changes for 6 months.

One option might be to install asdf-astropy and require it for building the docs on v5.3.x and v5.2.x (and update the documentation for the modeling example to describe that asdf-astropy is now required for saving astropy models to a file). Similar changes appear to have been included in astropy/astropy#14668

@pllim
Copy link
Contributor

pllim commented May 9, 2023

That would be too confusing for astropy users, as we have not removed deprecated code yet in those branches. I can always pin maxversion of asdf for those branches but I have a feeling that @nden won't like that.

@pllim
Copy link
Contributor

pllim commented May 9, 2023

Also, what is the rush of removing code here so much so soon?

@braingram
Copy link
Contributor Author

That would be too confusing for astropy users, as we have not removed deprecated code yet in those branches. I can always pin maxversion of asdf for those branches but I have a feeling that @nden won't like that.

I must be missing something. How would adding a dependency for building the docs be confusing to an astropy user?

@pllim
Copy link
Contributor

pllim commented May 9, 2023

We are breaking the promise of requiring something that should not be required.

@braingram
Copy link
Contributor Author

We are breaking the promise of requiring something that should not be required.

Serializing an astropy model with asdf 3.0 requires asdf-astropy. Are you proposing that asdf continue to support astropy.io.misc.asdf (which uses the deprecated asdf extension api) in asdf 3.0+? If so, how long would support for the deprecated extension api be required?

@pllim
Copy link
Contributor

pllim commented May 9, 2023

Didn't y'all write a whole long-term support policy a few months ago? I thought downstream would have another year, which wouldn't be a problem for us by then because astropy 6.0 should come out in 6 months.

@pllim
Copy link
Contributor

pllim commented May 9, 2023

It would be very easy for me to pin asdf<3 for astropy 5.x series, but it means STScI cannot use asdf 3 with stable astropy for another 6 months.

@braingram
Copy link
Contributor Author

Didn't y'all write a whole long-term support policy a few months ago? I thought downstream would have another year, which wouldn't be a problem for us by then because astropy 6.0 should come out in 6 months.

It was agreed that ASDF does not have the resources to support formal LTS releases. Would committing to supporting 2.15 for 6 months meet your needs?

The currently released version of ASDF (2.15) should allow the doctest you linked to pass. However the run you linked installed the 3.0 development branch (main) of ASDF. This will continue to fail even if we can agree to support 2.15.

It would be very easy for me to pin asdf<3 for astropy 5.x series, but it means STScI cannot use asdf 3 with stable astropy for another 6 months.

Is that the solution you prefer?

@pllim
Copy link
Contributor

pllim commented May 9, 2023

Is that the solution you prefer?

If you cannot revert this, then that is what I must do. It is wrong to introduce breaking changes in v5.3.x after the freeze. We are already in RC.

Thanks for your inputs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development No backport required Downstream CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants