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

add AsdfDeprecationWarning to AsdfFile.blocks #1336

Merged

Conversation

braingram
Copy link
Contributor

Public use of the block manager will be removed in ASDF 3.0 to allow for major changes to it's API. It is currently used by the legacy extensions (also removed in ASDF 3.0). The new converters do not currently have block storage access.

To provide a sense for possible breaking changes to the block manager and block storage access, the current experimental branch that adds block storage access to converters passes all block manager calls through the serialization context:
4f62156
Adding block storage access to the converter is scheduled for ASDF 3.0 and the API and implementation details are not yet finalized.

Fixes issue #1335

@braingram braingram added this to the 2.15 milestone Jan 30, 2023
@braingram
Copy link
Contributor Author

The error in asdf-astropy is interesting and may be a bug in asdf.
https://github.com/asdf-format/asdf/actions/runs/4046535979/jobs/6959444733

The tests triggered by the fits schema:
https://github.com/astropy/asdf-astropy/blob/main/asdf_astropy/resources/schemas/fits/fits-1.0.0.yaml
result in an attempt to serialize and deserialize an object with tag tag:astropy.org:astropy/fits/fits-1.0.0. This correctly uses to_yaml_tree and from_yaml_tree from the converter in asdf-astropy. However, the astropy.io.misc.asdf uses the same tag for FitsType which defines a reserve_blocks (I'm not sure why it needs to reserve blocks). https://github.com/astropy/astropy/blob/070fc3ce0e5bfeeecc0582844dc8a968f254dd7a/astropy/io/misc/asdf/tags/fits/fits.py#L68-L72
This results in FitsType.reserve_blocks being called on an object that is then serialized with AstropyFitsConverter.

It seems to me that finding a converter should prevent the use of the type similar to what is done during serialization:

asdf/asdf/yamlutil.py

Lines 256 to 266 in da32b40

if extension_manager.handles_type(type(obj)):
return _convert_obj(obj)
tag = ctx.type_index.from_custom_type(
type(obj),
ctx.version_string,
_serialization_context=_serialization_context,
)
if tag is not None:
return tag.to_tree_tagged(obj, ctx)

but no such check is done:

asdf/asdf/block.py

Lines 558 to 562 in da32b40

for node in treeutil.iter_tree(tree):
hook = ctx.type_index.get_hook_for_type("reserve_blocks", type(node), ctx.version_string)
if hook is not None:
for block in hook(node, ctx):
reserved_blocks.add(block)

@WilliamJamieson
Copy link
Contributor

However, the astropy.io.misc.asdf uses the same tag for FitsType which defines a reserve_blocks (I'm not sure why it needs to reserve blocks). astropy/astropy@070fc3c/astropy/io/misc/asdf/tags/fits/fits.py#L68-L72 This results in FitsType.reserve_blocks being called on an object that is then serialized with AstropyFitsConverter.

This is irrelevant as astropy.io.misc.asdf is deprecated, and we have already agreed with astropy that astropy.io.misc.asdf will not support ASDF 3.0. Thus we don't need to bend over backwards to support it. Astropy can be specifically updated to skip the ASDF related tests for ASDF < 3 and ignore the deprecation warnings for ASDF 2.15.

@braingram
Copy link
Contributor Author

This is irrelevant as astropy.io.misc.asdf is deprecated, and we have already agreed with astropy that astropy.io.misc.asdf will not support ASDF 3.0. Thus we don't need to bend over backwards to support it. Astropy can be specifically updated to skip the ASDF related tests for ASDF < 3 and ignore the deprecation warnings for ASDF 2.15.

That all sounds good. I opened a PR to fix the issue as it looks like a bug in asdf where it shouldn't be calling reserve_blocks on an object that will be serialized with a converter.

I looked into ignoring the warnings for specific astropy tests and I think the following should suffice if we're happy with the format of the warnings:
astropy/astropy@460f2bd

Public use of the block manager will be removed in ASDF 3.0
to allow for major changes to it's API. It is currently used
by the legacy extensions (also removed in ASDF 3.0). The new
converters do not currently have block storage access.

Fixes issue asdf-format#1335
tox.ini Outdated Show resolved Hide resolved
@braingram braingram marked this pull request as ready for review February 3, 2023 15:10
@braingram braingram requested a review from a team as a code owner February 3, 2023 15:10
@@ -136,7 +136,7 @@ commands_pre=
pip install -r {envtmpdir}/requirements.txt
pip freeze
commands=
pytest astropy/astropy/io/misc/asdf --open-files --run-slow --remote-data
pytest astropy/astropy/io/misc/asdf --open-files --run-slow --remote-data -W "ignore:The property AsdfFile.blocks has been deprecated:asdf.exceptions.AsdfDeprecationWarning:asdf.asdf:661"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilliamJamieson What are your thoughts on this as a way to keep our downstream CI from failing until astropy is updated to ignore this new deprecation?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine

Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

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

This might be out of scope, but as we add these deprecation warnings, we should create a distinct page in the docs which calls out all of them. We may want to include notes on timelines for removal (maybe link to mitigation strategies too).

Comment on lines +422 to +425
def test_blocks_deprecated():
af = AsdfFile()
with pytest.deprecated_call():
af.blocks
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion, more than a request maybe we should have a test_deprecated.py for this. We are going to have a bunch more deprecation warnings that we are going to have to test. It would be nice if those were all in one spot so that we can strip them out when they are done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent idea. I opened an issue to track this:
#1346

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

Successfully merging this pull request may close these issues.

2 participants