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

Update hdfwriter to include full filepath in root key in resource doc, add uuid DirectoryProvider for dynamic filenames #154

Closed
wants to merge 29 commits into from

Conversation

jwlodek
Copy link
Member

@jwlodek jwlodek commented Mar 15, 2024

This PR does the following:

  • Includes full filepath from the PandA hdf writer into the stream resource document. Before this the root key was hard coded to /
  • Removes hard-coded period and device name in PandA writer filenames. I think if you want to include that in the name adding it to the passed in DirectoryProvider is a better solution.
  • Adds a UUIDDirectoryProvider that generates dynamic filenames based on uuid4 - this is how we typically run things at NSLS-II.

…es, remove hard coded device name/period in panda writer, fix formatting
@jwlodek jwlodek changed the title Update hdfwriter to include full filepath in root key in resource doc Update hdfwriter to include full filepath in root key in resource doc, add uuid DirectoryProvider for dynamic filenames Mar 15, 2024
Copy link
Member

@mrakitin mrakitin left a comment

Choose a reason for hiding this comment

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

We tested this addition at our lab with real PandA. All worked great!

self.hdf.file_name.set(
f"{info.prefix}.{self.panda_device.name}{info.suffix}.h5"
),
self.hdf.file_name.set(f"{info.prefix}{info.suffix}.h5"),
Copy link
Member

Choose a reason for hiding this comment

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

A note to reviewers: this change was to exclude the mandatory PandA device name from the file name. If needed, this can be added via info.prefix at the device instantiation step.

@mrakitin
Copy link
Member

The tests/linter issues need to be resolved, of course.

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Mar 18, 2024

This is handled slightly differently with the exisitng HDFFileWriter (passing the DirectoryInfo in whole), maybe allowing us to refactor the writer to use Path signals if #122 gets merged. It'd be good to settle on a common handling (not opposed to changing what's already in) or merging the implementations of HDFWriter, _HDFFile

I've added a comment to the PR this PR is merging into to the effect of uniting the handling.

@coretl
Copy link
Collaborator

coretl commented Mar 18, 2024

@mrakitin why did you need to remove the PandA name from the HDF file path? We use the same DirectoryProvider in multiple detectors, so they need some level of uniqueness, hence the device name in the path.

An alternative would be to change the signature of DirectoryProvider so you would __call__ it with Device.name and a file extension, and it would make the whole Path for you...

@jwlodek
Copy link
Member Author

jwlodek commented Mar 18, 2024

I was going to include a DirectoryProvider that would append Device.name to the filename prefix with __call__ to this PR. Should I add that back in? My only hangup was what it should be called - I had originally used StaticDirectoryProviderWithDeviceName, but that seems quite a bit too long of a name. Either way, I think that is a cleaner solution than hard-coding the addition of the device name to the filename in the writer.

At NSLS-II we typically just use a UUID for the filename, which is inherently unique, so having a device name in the filepath is not necessary (see the UUIDDirectoryProvider) class included in this PR

@coretl
Copy link
Collaborator

coretl commented Mar 18, 2024

I was going to include a DirectoryProvider that would append Device.name to the filename prefix with __call__ to this PR. Should I add that back in? My only hangup was what it should be called - I had originally used StaticDirectoryProviderWithDeviceName, but that seems quite a bit too long of a name.

Could pass an enum to the __init__ to specify the file naming behaviour? StaticDirectoryProvider("/tmp/files", FileNaming.UID)

@coretl
Copy link
Collaborator

coretl commented Mar 19, 2024

@callumforrester @DiamondJoseph could we rename to FilePathProvider?

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Mar 19, 2024

I'm not precious about naming. There might be cause at this point to split FileNamer and DirectoryProvider into different things, with our implementation of FileNamer giving {beamline}-{name}_{data_collection} (even as I cling desperately to {prefix}{name}{suffix}) and NSLS can have a UUIDNameProvider while we share the StaticDirectoryProvider until we have need for our VisitDirectoryProvider sometime soon.

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.

6 participants