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.

Sign up for GitHub

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 duplicate validation, replace assumed validation on AsdfFile.__init__ with AsdfFile.validate #301

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Remove duplicate validation, replace assumed validation on AsdfFile.__init__ with AsdfFile.validate #301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c6088d
remove duplicate validation in to_asdf
braingram Jan 8, 2024
8f1e285
replace assumed validation in AsdfFile.__init__ with AsdfFile.validate
braingram Jan 8, 2024
8bfd39a
add changelog
braingram Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@

- Add ``meta.wcs`` to ``maker_utils``. [#302]

- Remove duplicate validation during ``DataModel.to_asdf``, replace assumed validation
during ``AsdfFile.__init__`` with call to ``AsdfFile.validate`` [#301]

0.18.0 (2023-11-06)
===================

Expand Down
13 changes: 9 additions & 4 deletions src/roman_datamodels/datamodels/_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ def wrapper(self, *args, **kwargs):
if self._asdf is None:
try:
with validate.nuke_validation():
self._asdf = asdf.AsdfFile({"roman": self._instance})
af = asdf.AsdfFile()
af["roman"] = self._instance
af.validate()
Comment on lines +42 to +44
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a bit verbose. Is there something wrong with:

self._asdf = asdf.AsdfFile({"roman": self._instance})
self._asdf.validate()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for taking a look and good question. Unfortunately, the code you proposed would validate the instance twice. Once during AsdfFile.__init__ and again during AsdfFile.validate. Also, the reason for this PR is that asdf is deprecating validation during AsdfFile.__init__. The code you proposed would result in a DeprecationWarning and test failure when the validation fails. The code proposed in this PR avoids the possibility of the DeprecationWarning by not providing the _instance during AsdfFile.__init__..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is asdf deprecating passing a dictionary directly to the AsdfFile initializer? If so, then that is a huge change of behavior!

If it's simply raising DepreciationWarning because of the validation change then a more thoughtful change should be considered as initializing with a dictionary is an extremely common usage of AsdfFile.

I would request that a validate=True option should be added to the initializer which preserves the current behavior without deprecation. You can set it so it defaults to None and raises the warning of impending behavior changes in that case.

Copy link
Collaborator Author

@braingram braingram Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

asdf is not deprecating initializing with a dictionary. However if that dictionary is invalid one will see a DeprecationWarning noting that validation on AsdfFile.__init__ is deprecated and a ValidationError. If the dictionary is valid no warning or error will be seen. This was done to allow test suites (like the one in roman_datamodels) to detect the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@WilliamJamieson I'm not sure I addressed your comments here. asdf is deprecating (for removal) validation on AsdfFile.__init__ to reduce the duplication of functionality in asdf. Adding a validate keyword that triggers validation overlaps with functionality that is already provided by AsdfFile.validate.

Copy link
Collaborator Author

@braingram braingram Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@WilliamJamieson does this PR look good to you? I'd like to resolve this so I can merge the PR in asdf adding the deprecation warning without breaking the CI here.

self._asdf = af
except ValidationError as err:
raise ValueError(f"DataModel needs to have all its data flushed out before calling {func.__name__}") from err

Expand Down Expand Up @@ -126,8 +129,10 @@ def __init__(self, init=None, **kwargs):
)
with validate.nuke_validation():
self._instance = init
self._asdf = asdf.AsdfFile({"roman": init})

af = asdf.AsdfFile()
af["roman"] = self._instance
af.validate()
self._asdf = af
return

if init is None:
Expand Down Expand Up @@ -220,7 +225,7 @@ def open_asdf(self, init=None, **kwargs):
def to_asdf(self, init, *args, **kwargs):
with validate.nuke_validation(), _temporary_update_filename(self, Path(init).name):
asdf_file = self.open_asdf(**kwargs)
asdf_file.tree = {"roman": self._instance}
asdf_file["roman"] = self._instance
asdf_file.write_to(init, *args, **kwargs)

def get_primary_array_name(self):
Expand Down
Loading