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

Jump to bottom

Parameters implicitly creating storage_directory on access #527

Open
rtuck99 opened this issue Sep 25, 2024 · 2 comments · May be fixed by #643
Open

Parameters implicitly creating storage_directory on access #527

rtuck99 opened this issue Sep 25, 2024 · 2 comments · May be fixed by #643
Assignees
@rtuck99
Labels
hyperion

Comments

@rtuck99
Copy link
Contributor

rtuck99 commented Sep 25, 2024

The detector_params property accessors in GridCommon and RotationScan create the storage_directory implicitly.

This violates the principle of least surprise. Also it means a filesystem access every time the property is accessed to check if the directory exists and create it if necessary.
The existence of the directory is asserted by DetectorParams.directory field validator (which is reasonable).
The directories should be created by the experiment plans that create the files. However many unit tests will also need to be updated as well.

See also #524 which was raised due to the code that cleans up after the unit tests having issues.
@DominicOram DominicOram moved this to Candidates for Next Sprint in Hyperion Oct 14, 2024
@DominicOram
Copy link
Contributor

If in validator it will get invoked on every deserialise so may be bad. Think harder about this

@rtuck99 rtuck99 self-assigned this Nov 11, 2024
@rtuck99
Copy link
Contributor Author

rtuck99 commented Nov 11, 2024

I think checking/creating on validate is definitely still better than on property access. However we may want to have a separate explicit runtime validation method.
I ran the load_centre_collect_full_plan system test and the property accessor was called 22 times, the model_validate method was called 4 times (excluding the test setup)

@rtuck99 rtuck99 linked a pull request Nov 12, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rtuck99 rtuck99

Labels
hyperion
Projects
Hyperion
Status: In Progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reduce filesystem access when validating parameters DiamondLightSource/mx-bluesky
2 participants
@rtuck99 @DominicOram