Skip to content

Add warning when conversion fails with default warn_on_failed_conversion - #2125

Merged
sydduckworth merged 16 commits into
asdf-format:mainfrom
sydduckworth:warn-on-warn-on-failed-conversion-2
Sep 2, 2026
Merged

Add warning when conversion fails with default warn_on_failed_conversion#2125
sydduckworth merged 16 commits into
asdf-format:mainfrom
sydduckworth:warn-on-warn-on-failed-conversion-2

Conversation

@sydduckworth

@sydduckworth sydduckworth commented Aug 28, 2026

Copy link
Copy Markdown
Member

Description

Part of #2127

  • Added is_set function which can be used to check if attributes of an object have been modified along with an _IsSet base class necessary for it to function
  • Added asdf.exceptions.AsdfFutureWarning
  • Updated file loading logic so that if node conversion fails and warn_on_failed_conversion hasn't been manually set then AsdfFutureWarning is emitted before an exception is raised.

PRs to address downstream test failures:

AI Disclosure

No AI tools used

Tasks

  • run prek on your machine
  • run pytest on your machine
  • Does this PR add new features and / or change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update relevant docstrings and / or docs/ page
    • for any new features, add unit tests
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: bug fix
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change

@sydduckworth
sydduckworth marked this pull request as ready for review August 31, 2026 17:29
@sydduckworth
sydduckworth requested a review from a team as a code owner August 31, 2026 17:29

@braingram braingram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for putting this together. I left a few suggestions.

What's your take/plan for the downstream errors?

Comment thread asdf/config.py
Comment thread asdf/util.py Outdated
Comment thread asdf/yamlutil.py Outdated
warnings.warn(f"A node failed to convert with: {err}", AsdfConversionWarning)
obj = node
else:
if not is_set(get_config()).warn_on_failed_conversion:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What are options for consolidating this code with the updates in lazy_nodes to avoid duplicating the error message?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Idk the message is only in two places and will be removed following the next major release, so to me it doesn't seem like a problem. If we really wanted to avoid duplication we could create an AsdfFutureWarning subtype with that text as its message, but that seems like more overhead than the current approach.

Comment thread asdf/config.py Outdated
@sydduckworth

Copy link
Copy Markdown
Member Author

@braingram the downstream errors are pretty simple to fix, generally they just require manually setting warn_on_failed_conversion = True in a few tests. I was going to put in an issue about the change for stdatamodels and asdf-astropy.
The downstream changes can be made without needing this PR to actually be merged/released, but I thought it might be good to wait to put the PRs in until this PR is at least approved to demonstrate that the upstream change is actually happening lol.

@braingram braingram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! This warning will hopefully inform some downstream users and the docs and details helpfully describe the upcoming changes and how code can be adapted.

@sydduckworth
sydduckworth merged commit cd968a4 into asdf-format:main Sep 2, 2026
65 of 67 checks passed
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