Skip to content

fix deepcopy of lazy tree#1922

Merged
braingram merged 2 commits into
asdf-format:mainfrom
braingram:copy_lazy_tree
May 20, 2025
Merged

fix deepcopy of lazy tree#1922
braingram merged 2 commits into
asdf-format:mainfrom
braingram:copy_lazy_tree

Conversation

@braingram
Copy link
Copy Markdown
Contributor

@braingram braingram commented May 8, 2025

Description

This fixes a bug where a deepcopy of a lazy node retains references to the original lazy tree (instead of becoming non-lazy nodes). While adding tests for and fixing this issue I found that walk_and_modify converts AsdfOrderedDictNode instances into dict. That bug is also fixed in this PR and is required for this PR since walk_and_modify is used to handle the deepcopy.

Tasks

  • run pre-commit 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

@braingram
Copy link
Copy Markdown
Contributor Author

braingram commented May 8, 2025

Docs failed due to temporary unavailability of the astropy objects.inv needed for intersphinx links. EDIT: fixed by a rerun

@braingram braingram marked this pull request as ready for review May 8, 2025 18:09
@braingram braingram requested a review from a team as a code owner May 8, 2025 18:09
Comment on lines +430 to +431
with asdf.open(fn, lazy_tree=True) as af:
af2 = af.copy()
Copy link
Copy Markdown
Member

@zacharyburnett zacharyburnett May 9, 2025

Choose a reason for hiding this comment

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

should we also test multiple files open in context managers? To replicate the original issue (or was that using file handles)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! The original issue can be seen with the following example:

import gc
import roman_datamodels.datamodels as rdm
dst = rdm.ImageModel()
with rdm.open("some_cal.asdf") as src:
    dst.meta = src.meta.copy()
del src
gc.collect(2)
dst.meta.ref_file  # ok since the copy is shallow
dst.meta.ref_file.crds  # fails due to the sub node `crds` not being resolved (made not lazy) during the copy

With this PR that example runs without failure.
What did you have in mind for the multiple files?
I tried to flush out this test with a few "gotchas" (the recursive reference, etc) but it's far from exhaustive. Thanks for the idea!

Copy link
Copy Markdown
Member

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

this LGTM as far as I can tell

@braingram braingram enabled auto-merge May 20, 2025 13:02
@braingram braingram merged commit b047e85 into asdf-format:main May 20, 2025
34 of 35 checks passed
@braingram braingram deleted the copy_lazy_tree branch May 20, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants