Allow for passing through windows drive letter to NSLS2PathProvider - #231
Merged
Thomas Hopkins (thopkins32) merged 11 commits intoSep 2, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the nslsii.ophyd_async providers to better support Windows-style paths by allowing a Windows drive letter to be passed through to NSLS2PathProvider, and it tightens typing in the associated tests.
Changes:
- Add
windows_drive_lettersupport toNSLS2PathProviderand refactor path generation to usePurePathsemantics. - Introduce new filename providers (
TimestampFilenameProvider,REMetadataFilenameProvider) and aRunEngineMetadatatype alias. - Add type annotations to the
test_nsls2_path_providertest function parameters.
File summaries
| File | Description |
|---|---|
| nslsii/ophyd_async/providers.py | Adds Windows drive-letter support, refactors directory/URI generation, and introduces timestamp/metadata-based filename providers. |
| nslsii/tests/ophyd_async/test_ophyd_async_providers.py | Adds type annotations to test parameters (no behavioral change). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
nslsii/ophyd_async/providers.py:4
Pathis imported but never used in this module, which can trip linters and adds noise. Remove the unused import.
from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
nslsii/ophyd_async/providers.py:236
- New behavior (
windows_drive_letterand thedirectory_urifield) is introduced here, but the existing tests only assert the POSIX-styledirectory_path. Add a unit test covering a Windows drive letter input and assert the resultingdirectory_path+directory_uriformatting.
windows_drive_letter: str | None = None,
nslsii/ophyd_async/providers.py:236
windows_drive_letteris now part of theNSLS2PathProviderpublic constructor, but it is not documented in the class docstring's Parameters section. This makes the new API hard to discover for users.
windows_drive_letter: str | None = None,
nslsii/ophyd_async/providers.py:139
- The class docstring mentions an
include_datakey_nameparameter, butTimestampFilenameProviderhas no constructor argument for that. This is misleading API documentation.
include_datakey_name : bool, default False
Whether to include the datakey name in the filename. If True, the datakey name will be prefixed to the filename.
"""
nslsii/ophyd_async/providers.py:170
- The
REMetadataFilenameProviderdocstring documentsmetadata_dictbut not the requiredformat_stringargument, and it also mentionsinclude_datakey_nameeven though no such parameter exists. This makes the public API confusing.
metadata_dict : dict
Typically `RE.md`. Used for dynamic save path generation from sync-d experiment
include_datakey_name : bool, default False
Whether to include the datakey name in the filename. If True, the datakey name will be prefixed to the filename.
"""
nslsii/ophyd_async/providers.py:249
windows_drive_letteris interpolated directly into the Windows path. If a caller passes'C:'(instead of'C'), this will produce an invalid path likeC::\\proposals. Consider normalizing/validating the input and reusing a single computed base dir for read/write.
self._base_read_directory = (
self.get_beamline_proposals_dir(beamline_tla=beamline_tla, beamline_tla_suffix=beamline_tla_suffix)
if not windows_drive_letter
else PureWindowsPath(f"{windows_drive_letter}:\\proposals")
)
nslsii/ophyd_async/providers.py:361
- When
windows_drive_letteris used,full_read_path.as_posix()yields something likeC:/...(no leading/). Passing that directly tourlunparsewithnetloc='localhost'produces an invalid file URI. Ensure the URL path starts with/for Windows drive paths.
directory_uri=urlunparse(
(
"file",
"localhost",
f"{full_read_path.as_posix()}/",
nslsii/ophyd_async/providers.py:158
TimestampFilenameProviderusesdate.today()with a format string that includes hours/minutes/seconds.datehas no time component, so this will always render00for%H%M%S, which breaks the intent of a timestamp-based filename.
timestamp = date.today().strftime("%Y%m%d_%H%M%S")
if datakey_name is not None:
return f"{datakey_name}_{timestamp}"
return timestamp
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
nslsii/ophyd_async/providers.py:4
Pathis imported but not used anywhere in this module. This can fail linting and makes the import list misleading; remove it.
This issue also appears in the following locations of the same file:
- line 234
- line 292
from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
nslsii/ophyd_async/providers.py:368
directory_uriis intended to be POSIX, but whenseparator="\\"the YMD portion is generated with backslashes and can leak into the URI path. Normalize to forward slashes when building the URI to avoid invalidfile://URIs.
directory_uri=urlunparse(
(
"file",
"localhost",
f"{full_read_path.as_posix()}/",
nslsii/ophyd_async/providers.py:169
- The
REMetadataFilenameProviderdocstring mentionsinclude_datakey_name, but the class does not accept such a parameter (it inherits the prefixing behavior fromTimestampFilenameProvider). The docstring also omits theformat_string/timestamp_formatparameters that are required by__init__.
"""Filename provider that generates filenames based on the current timestamp and the RE metadata.
Parameters
----------
metadata_dict : dict
nslsii/ophyd_async/providers.py:294
- In
generate_directory_paththe docstring describesdatakey_nameas required (str), but the signature allowsNone. Mark it optional to match the function signature and behavior.
datakey_name : str
The name of the datakey to include in the path.
If provided, the datakey name will be used as a prefix to the YMD portion of the path.
nslsii/ophyd_async/providers.py:238
windows_drive_letterchanges the meaning/type ofdirectory_path(Windows drive path vs/nsls2/data/...), but this new parameter is not documented in theNSLS2PathProviderdocstring parameter list. Add it so callers understand the read/write split.
def __init__(
self,
metadata_dict: RunEngineMetadata,
filename_provider: FilenameProvider = UUIDFilenameProvider(),
nslsii/ophyd_async/providers.py:151
- The
TimestampFilenameProvider.__call__docstring mentionsinclude_datakey_name, but there is no such flag; the implementation prefixes wheneverdatakey_nameis provided. Update the docstring line to match behavior.
datakey_name : str, optional
The name of the datakey to include in the filename. Only used if include_datakey_name is True.
nslsii/tests/ophyd_async/test_ophyd_async_providers.py:107
- The test states the read URI always uses POSIX paths, but it doesn't assert that no backslashes appear in the URI (which can happen when
separator="\\"). Adding a regression assertion here will catch URI formatting issues for Windows-style separators.
# Read URI always uses POSIX paths
assert info.directory_uri.startswith(
f"file://localhost/nsls2/data/{tla_full}/proposals/2024-3/pass-000000/assets/test"
)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
nslsii/ophyd_async/providers.py:4
Pathis imported frompathlibbut never used in this module, which will trigger unused-import linting and adds noise.
from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
nslsii/ophyd_async/providers.py:174
- This class docstring lists
include_datakey_name, but the constructor takesformat_string,timestamp_format, andmetadata_dict. The parameters section should reflect the actual signature to avoid confusing users.
metadata_dict : dict
Typically `RE.md`. Used for dynamic save path generation from sync-d experiment
include_datakey_name : bool, default False
Whether to include the datakey name in the filename. If True, the datakey name will be prefixed to the filename.
nslsii/ophyd_async/providers.py:240
windows_drive_letteris a new public initializer parameter but it is not documented in the class docstring’s Parameters section. Please add it so users know how write-path vs read-URI behavior changes when it is set.
windows_drive_letter: str | None = None,
nslsii/ophyd_async/providers.py:256
windows_drive_letteris interpolated directly into a Windows path. If a caller passes an empty string or something other than a single A–Z letter, this will silently create invalid paths. Consider validating/normalizing it (and uppercasing) before constructingPureWindowsPath.
self._base_write_directory = (
self._base_read_directory
if not windows_drive_letter
else PureWindowsPath(f"{windows_drive_letter}:\\proposals")
)
nslsii/ophyd_async/providers.py:192
- The docstring references
include_datakey_name, butREMetadataFilenameProvider.__call__always prefixesdatakey_namewhen provided (via the base class). This should be updated to match behavior.
The name of the datakey to include in the filename. Only used if include_datakey_name is True.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a likely breaking public API change in NSLS2PathProvider (removed/renamed constructor parameters) and contains docstring inaccuracies that should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
nslsii/ophyd_async/providers.py:58
- The TimestampFilenameProvider docstring mentions an
include_datakey_nameflag that doesn't exist; the behavior is simply thatdatakey_nameis optionally prefixed to the timestamp.
datakey_name : str, optional
The name of the datakey to include in the filename. Only used if include_datakey_name is True.
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
There are concrete correctness/documentation/test issues in the changed code (notably drive-letter parsing permissiveness and mismatched docstrings/test assertions) that should be addressed before approval.
Review details
Suppressed comments (7)
Previously missed (4) — in code that hasn't changed since the last review.
nslsii/tests/ophyd_async/test_ophyd_async_providers.py:76
- This parametrization no longer includes a non-Windows case that passes a custom
separator, so the POSIX-path behavior for custom separators isn’t exercised by tests.
nslsii/tests/ophyd_async/test_ophyd_async_providers.py:126 - For POSIX paths,
effective_sepis currently forced toos.path.sep, which makes it impossible to assert behavior when a customseparatoris provided. Useymd_separator or os.path.sepso assertions match the actual configured separator.
nslsii/ophyd_async/providers.py:58 - Docstring refers to
include_datakey_name, but this provider always prefixesdatakey_namewhen it is provided (there is noinclude_datakey_nameflag). Update the docstring to match the actual behavior to avoid confusing API consumers.
This issue also appears on line 177 of the same file.
nslsii/ophyd_async/providers.py:136
- The helper returns a Windows drive root path (e.g. "C:\"), not just a string that "ends with a colon". Updating the docstring wording will make the intent clearer.
This issue also appears on line 149 of the same file.
nslsii/ophyd_async/providers.py:186
windows_drive_letteris part of the public initializer signature but is not documented in the class docstring parameters, and the docstring doesn’t mention that YMDGranularity.none is supported. This makes the API harder to discover and can lead to misuse.
beamline_data_dirname : str, optional
Name of the beamline data directory to use in the path, typically lowercase beamline TLA.
If not provided, the name will be determined from the ENDSTATION_ACRONYM or
BEAMLINE_ACRONYM environment variables.
separator : str, optional
Separator to use in YMD portion of the path. Defaults to default path separator given write directory semantics.
granularity : YMDGranularity | str, default YMDGranularity.day
Granularity of the YMD portion of the path. If set to YMDGranularity.day, the path will include year, month,
and day directories. If set to YMDGranularity.month, the path will include year and month directories.
If set to YMDGranularity.year, the path will include only the year directory.
nslsii/ophyd_async/providers.py:153
- The current normalization silently truncates anything after "C:" (e.g. passing "C:\data" becomes just "C"), which can hide caller mistakes. Consider only accepting "C", "C:", or "C:\" and raising ValueError for any longer/other forms.
# If more than one character is provided and the second character is a colon,
# take only the first character
if len(drive_letter) > 1 and drive_letter[1] == ":":
drive_letter = drive_letter[0]
nslsii/ophyd_async/providers.py:114
- Docstring refers to
include_datakey_name, but the actual behavior is controlled byprepend_datakey_name. This mismatch can mislead users of the API; update the parameter description accordingly.
"""Generates a filename based on the current timestamp and the RE metadata.
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Thomas Hopkins (thopkins32)
approved these changes
Sep 2, 2026
Thomas Hopkins (thopkins32)
left a comment
Thomas Hopkins (thopkins32)
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Just want to check one assumption about Windows drive letter and how it maps to a directory in practice.
Jakub Wlodek (jwlodek)
requested review from
Copilot and
Thomas Hopkins (thopkins32)
September 2, 2026 16:35
Thomas Hopkins (thopkins32)
approved these changes
Sep 2, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and robustness issues in the new provider logic (year granularity formatting, granularity error handling, base dir input normalization) plus test environment leakage that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
nslsii/ophyd_async/providers.py:183
- NSLS2PathProvider accepts
granularityas a string, but invalid values currently raise a KeyError from enum lookup; this should be a ValueError with a clear message.
nslsii/ophyd_async/providers.py:243 - For
YMDGranularity.year, the date template includes the separator (e.g."%Y_"), which produces directory names like"2026_"whenseparator="_"; year-only directories should not include a trailing separator.
nslsii/ophyd_async/providers.py:58
- The TimestampFilenameProvider docstring mentions an
include_datakey_nameflag that doesn't exist; this is misleading for callers reading the API docs.
Parameters
----------
datakey_name : str, optional
The name of the datakey to include in the filename. Only used if include_datakey_name is True.
nslsii/ophyd_async/providers.py:120
- The REMetadataFilenameProvider call docstring also references an
include_datakey_nameoption that is not part of this class; update the parameter description to match actual behavior (prepend_datakey_name).
Parameters
----------
datakey_name : str, optional
The name of the datakey to include in the filename. Only used if include_datakey_name is True.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+187
to
+200
| if not base_data_dir: | ||
| tla = os.getenv("ENDSTATION_ACRONYM", os.getenv("BEAMLINE_ACRONYM", "")).lower() | ||
| if not tla: | ||
| raise ValueError( | ||
| "Neither ENDSTATION_ACRONYM nor BEAMLINE_ACRONYM environment variables are set. " | ||
| "Please set one of these environment variables or provide a base_data_dir." | ||
| ) | ||
| self._base_data_dir = PurePosixPath(f"/nsls2/data/{tla}/proposals") | ||
| else: | ||
| self._base_data_dir = base_data_dir | ||
|
|
||
| self._base_write_dir = self._base_data_dir if not base_write_dir else base_write_dir | ||
|
|
||
| self._ymd_separator = separator or ("\\" if isinstance(self._base_write_dir, PureWindowsPath) else "/") |
Comment on lines
67
to
77
| os.environ["BEAMLINE_ACRONYM"] = "tst" | ||
|
|
||
| pp = NSLS2PathProvider( | ||
| dummy_re_md_dict, | ||
| filename_provider=static_fp, | ||
| beamline_tla=tla_override, | ||
| beamline_tla_suffix="-new" if with_suffix else None, | ||
| base_data_dir=base_data_dir, | ||
| granularity=ymd_granularity, | ||
| separator=ymd_separator, | ||
| include_scan_id_dir=include_scan_id_dir, | ||
| base_write_dir=base_write_dir, | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the
NSLS2PathProviderto support passing in a separate write directory. Also simplifies the logic for the base directory setup. Removes theAcqModeFilenameProvider, replaced instead by theREMetadataFilenameProviderthat allows for creating filenames from formatted RE metadata.