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

Move array storage settings to config #1468

Merged

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Mar 8, 2023

This PR moves:

  • all_array_storage
  • all_array_compression
  • all_array_compression_kwargs

into asdf.config.AsdfConfig. These were previously passed around as arguments to several functions during AsdfFile writes. By moving them into the config the AsdfFile can continue to accept them as arguments to AsdfFile.write_to and AsdfFile.update (by using the arguments to set the values in the AsdfConfig context created within those functions) and now users can also adjust these settings in the config directly.

This helps to make these storage settings available to all functions called within the AsdfConfig contexts created during write without having to pass them around as arguments (this proved important for the upcoming deferred block and Converter block storage support).

@braingram braingram added the development No backport required label Mar 8, 2023
@github-actions github-actions bot added this to the 3.0.0 milestone Mar 8, 2023
@braingram braingram force-pushed the feature/config_storage_settings branch 3 times, most recently from 723f51c to f387daf Compare March 8, 2023 19:52
@braingram braingram marked this pull request as ready for review March 8, 2023 19:52
@braingram braingram requested a review from a team as a code owner March 8, 2023 19:52
@braingram braingram force-pushed the feature/config_storage_settings branch 3 times, most recently from f3db344 to 07cb824 Compare March 10, 2023 21:35
@braingram braingram force-pushed the feature/config_storage_settings branch from 07cb824 to 8b0c995 Compare March 22, 2023 14:30
@perrygreenfield
Copy link
Contributor

Let me see if I understand correctly. The main advantage is to remove the necessity of passing ctx as an argument to many of these functions to clean up the interface? Otherwise the changes look good to me. I just want to make sure I understand the the goal.

@braingram
Copy link
Contributor Author

Let me see if I understand correctly. The main advantage is to remove the necessity of passing ctx as an argument to many of these functions to clean up the interface? Otherwise the changes look good to me. I just want to make sure I understand the the goal.

Exactly!

It also allows everything within the context to have access to the setting. For example the eventual ndarray converter will benefit from access to these settings during a call to to_yaml_tree. Currently to_yaml_tree only receives a SerializationContext (as the ctx argument) and not a reference to the AsdfFile (where the settings are located prior to this PR). With this PR the all_array_storage (etc) settings are available within the scope of the call to Converter.to_yaml_tree.

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram force-pushed the feature/config_storage_settings branch from 8b0c995 to fb08e2e Compare March 24, 2023 14:44
@braingram
Copy link
Contributor Author

LGTM

Thanks for reviewing this. I rebased it to main and will enable auto-merge.

@braingram braingram merged commit 8dce02c into asdf-format:main Mar 24, 2023
@braingram braingram deleted the feature/config_storage_settings branch March 24, 2023 15:10
self._all_array_storage = value

@property
def all_array_compression(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

@braingram sorry to ceiling-cat this closed PR, but it occurred to me that this is a nice opportunity to combine the compression-related setters into a single method call. It seems odd that our interface forces us to set them separately, since afaik they will always go together, and if we change the compression algorithm we probably should at least wipe out the kwargs from the previous algorithm.

Also, wdyt about changing the name and semantics of these config item to be "default" instead of "all"? The "all" can be an awkward tool sometimes, since you can't modify the compression of individual arrays if you use an "all" setting. For example, if your AsdfFile has 100 arrays and you want to use blosc for 99 of them and no compression for the remaining array, all_array_compression isn't useful, instead you have to call AsdfFile.set_array_compression on all 99.

Anyway no pressure to make changes, I just had to get out some ideas that have been in the back of my head about these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those all sounds like good improvements to me. I opened an issue to track the changes:
#1503
Given that this PR won't be released until the first 3.0 release, I milestoned the issue for 3.0 so we can make the change to the config api before it's made public (so no deprecation warning should be needed, although we will need to deprecate the arguments to open).

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

Successfully merging this pull request may close these issues.

None yet

3 participants