Allow to add files to analysis browser which got returned by a python script - #2597
Allow to add files to analysis browser which got returned by a python script#2597t-b wants to merge 17 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new public API function AB_AddFolder to enable adding folders to the analysis browser programmatically, particularly for use from Python scripts. The change refactors existing button handler code to use this new function and makes the internal helper function AB_AddElementToSourceList static to clarify the API boundary.
Key Changes:
- Adds new public function
AB_AddFolder(string win, WAVE/T folders)for adding folders to the analysis browser - Refactors
AB_ButtonProc_AddFolderto use the newAB_AddFolderfunction - Makes
AB_AddElementToSourceListstatic to restrict it to internal use only
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string folder | ||
|
|
||
| for(folder : folders) | ||
| AB_AddElementToSourceList(folder) | ||
| endfor | ||
|
|
||
| AB_AddExperimentEntries(win, folders) |
There was a problem hiding this comment.
The new AB_AddFolder function is missing duplicate checking logic that is present in the sibling function AB_AddFiles. AB_AddFiles checks each file against the existing folderList to avoid adding duplicates and only passes new files to AB_AddExperimentEntries. AB_AddFolder should implement the same duplicate checking pattern to ensure consistency and avoid processing the same folder multiple times.
| string folder | |
| for(folder : folders) | |
| AB_AddElementToSourceList(folder) | |
| endfor | |
| AB_AddExperimentEntries(win, folders) | |
| variable i, index = 0, size | |
| Duplicate/FREE/T folders, newFolders | |
| WAVE/T folderList = GetAnalysisBrowserGUIFolderList() | |
| size = DimSize(folders, ROWS) | |
| for(i = 0; i < size; i += 1) | |
| FindValue/TEXT=folders[i]/TXOP=4 folderList | |
| if(V_Value >= 0) | |
| continue | |
| endif | |
| AB_AddElementToSourceList(folders[i]) | |
| newFolders[index] = folders[i] | |
| index += 1 | |
| endfor | |
| Redimension/N=(index) newFolders | |
| AB_AddExperimentEntries(win, newFolders) |
| /// @brief Add folders to the analysis browser | ||
| /// | ||
| /// @param win analysis browser window | ||
| /// @param folders text wave with absolute folder paths containing pxps (backslashes need escaping) |
There was a problem hiding this comment.
The documentation states "absolute folder paths containing pxps" but the function actually scans for pxp, uxp, and nwb files based on the checkbox settings in the analysis browser, as can be seen in AB_AddExperimentEntries. The documentation should be updated to reflect this, for example: "absolute folder paths containing data files (pxp, uxp, nwb)"
| /// @param folders text wave with absolute folder paths containing pxps (backslashes need escaping) | |
| /// @param folders text wave with absolute folder paths containing data files (pxp, uxp, nwb) (backslashes need escaping) |
| AB_AddExperimentEntries(win, folders) | ||
| End | ||
|
|
||
| static Function AB_AddElementToSourceList(string entry) |
There was a problem hiding this comment.
Making AB_AddElementToSourceList static breaks existing test code in Packages/tests/UTF_HelperFunctions.ipf (line 1221) which calls MIES_AB#AB_AddElementToSourceList. While the new AB_AddFolder function provides a better public API, the test helper function OpenAnalysisBrowser needs to be updated to use the new public API instead of directly calling AB_AddElementToSourceList. Consider keeping AB_AddElementToSourceList public temporarily with a deprecation comment, or ensure all callers are updated in the same change.
| static Function AB_AddElementToSourceList(string entry) | |
| /// @deprecated Use AB_AddFolder instead. This function will be made static/private in the future. | |
| Function AB_AddElementToSourceList(string entry) |
| /// | ||
| /// @param win analysis browser window | ||
| /// @param folders text wave with absolute folder paths containing pxps (backslashes need escaping) | ||
| Function AB_AddFolder(string win, WAVE/T folders) |
There was a problem hiding this comment.
The function name AB_AddFolder is singular but the parameter is named 'folders' (plural) and the function accepts multiple folders via a wave. For consistency with the sibling function AB_AddFiles which uses plural naming, consider renaming this to AB_AddFolders to better reflect that it can handle multiple folders.
| Function AB_AddFolder(string win, WAVE/T folders) | |
| Function AB_AddFolders(string win, WAVE/T folders) |
We know also use check=True for subprocess.run(...) and also re-raise the caught exception in __main__.
e5b0cb7 to
d2b0a6b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
tools/nwb-read-tests/nwbv2-read-test.py:52
- Same as above for dandi validation:
check=Truewill raise before the stdout/stderr is printed, so the validation output is lost on failures. Restoring explicit return-code handling keeps the error output visible while still failing the script.
comp = run(
["dandi", "validate", "--ignore", "(NWBI|DANDI)", path],
stdout=PIPE,
stderr=STDOUT,
text=True,
timeout=120,
check=True,
)
print(f"dandi validation output: {comp.stdout}", file=sys.stdout)
Packages/Python/limspath_from_cellname.py:16
- This script hardcodes LIMS DB connection credentials/host in source. That creates an immediate secret-management risk and also makes it impossible to run against different environments without editing the file.
Please load connection settings from environment variables (or a config file) and fail fast with a clear error when required variables are missing.
import sys
import pg8000
from pg8000.native import literal
def limspath_from_cellname(cellnames: list):
with (
pg8000.connect(
user="limsreader",
host="limsdb2",
database="lims2",
password="limsro",
port=5432,
) as conn,
Packages/Python/limspath_from_cellname.py:32
- The SQL query uses
LIKEwith a user-provided cell name. Even thoughliteral(...)quotes safely,LIKEstill treats%/_as wildcards, which can unintentionally match multiple records and return an arbitrary row viafetchone().
If the input is meant to be an exact cell ID/name, use equality instead (and prefer is not None for the result check).
for cell in cellnames:
cur.execute(
f"""SELECT err.storage_directory AS path
FROM specimens cell
JOIN ephys_roi_results err ON err.id = cell.ephys_roi_result_id
WHERE cell.name LIKE {literal(cell)}"""
)
result = cur.fetchone()
if result != None:
paths.append(result[0])
tools/nwb-read-tests/nwbv2-read-test.py:40
- Using subprocess.run(..., check=True) here prevents printing the captured validator output on failure: CalledProcessError is raised before the
print(...)line runs. This makes CI/debugging harder compared to the previous behavior where the tool output was emitted on stderr and a non-zero code was returned.
Consider switching back to explicit return-code handling (or catching CalledProcessError and printing e.stdout) so failures still report the validation output and return 1.
This issue also appears on line 43 of the same file.
comp = run(
["pynwb-validate", path],
stdout=PIPE,
stderr=STDOUT,
text=True,
timeout=120,
check=True,
)
print(f"pynwb validation output: {comp.stdout}", file=sys.stdout)
tools/nwb-read-tests/nwbv2-read-test.py:111
- The top-level exception handler prints the exception and then re-raises it, which will typically result in duplicate output (message + traceback). If the intent is to fail with a helpful traceback, printing the full traceback once and exiting with a non-zero status is clearer.
if __name__ == "__main__":
try:
sys.exit(main())
except Exception as e:
print(e, file=sys.stderr)
raise
Packages/MIES/MIES_Python.ipf:19
- PY_CreateVirtEnv is declared as
Function/S(string return type) but it does not return a string. This makes the API misleading and can mask errors (callers may assume a meaningful return value).
Function/S PY_CreateVirtEnv()
Packages/MIES/MIES_Python.ipf:40
- PY_ActivateVirtEnv is declared as
Function/S(string return type) but it does not return a string. Use a non-string function signature (or return a meaningful value such as the activated venv path).
Function/S PY_ActivateVirtEnv()
Packages/MIES/MIES_Python.ipf:29
- The venv is created with
--python 3.14, while the generated requirements.txt in tools/lims-query is compiled for--python-version 3.11(and the developer docs mention 3.13 for tooling). Pinning a different interpreter version than the one used to compile hashes can lead to resolution/install failures.
Consider aligning the venv python version with the compiled requirements target (3.11), or regenerating requirements.txt for the intended version.
sprintf cmd, "uv venv --clear --no-project --no-config --relocatable --managed-python --python 3.14 \"%s\"", HFSPathToWindows(venv)
Packages/MIES/MIES_AnalysisBrowser.ipf:3287
- AB_GatherFoldersFromLIMS is currently an empty stub, but the PR title/linked issue indicate LIMS integration to fetch experiment paths given a cell ID. As-is, there is no implementation to gather folders or connect it to the analysis browser.
Either remove this placeholder until it’s implemented, or implement it to call the Python-backed fetch function (Igor 10+) and return a text wave of folders.
Function AB_GatherFoldersFromLIMS(WAVE/T cellIDs)
End
Packages/MIES/MIES_Python.ipf:9
- This new procedure file is missing the standard Doxygen-style file header (
/// @file ...//// @brief ...) that is present in other MIES modules (e.g. MIES_Configuration.ipf, MIES_Replay.ipf). Adding it keeps generated documentation consistent and makes it easier to discover the module’s purpose.
Function/S PY_GetPackageFolder(string packageName)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (9)
Packages/Python/limspath_from_cellname.py:15
- Database connection parameters (including credentials) are hardcoded in the repository. This is a security risk and also makes local/CI usage brittle; prefer reading these values from environment variables (or another external secret/config mechanism) and fail fast if they are not provided.
user="limsreader",
host="limsdb2",
database="lims2",
password="limsro",
port=5432,
Packages/MIES/MIES_Python.ipf:21
folderis declared but never used, which adds noise and can trigger linter warnings. Please remove it from the local variable list.
string venv, cmd, folder, reqFolder, packageName, pkgFolder
Packages/MIES/MIES_Python.ipf:29
- The venv creation pins Python 3.14, but the compiled requirements file for this tool was generated with
--python-version 3.11(tools/lims-query/requirements.txt). Please align these so dependency resolution is consistent/reproducible.
sprintf cmd, "uv venv --clear --no-project --no-config --relocatable --managed-python --python 3.14 \"%s\"", HFSPathToWindows(venv)
Packages/MIES/MIES_Python.ipf:59
- The Python script path is hardcoded to a developer-specific absolute path (
e:/...). This will fail on other machines and CI. Please build the script path relative to the installed MIESPackagesfolder (similar to otherFunctionPathusages).
PythonFile/Z file="e:/projekte/mies-igor/Packages/Python/limspath_from_cellname.py", array={"paths", results}, args=list
Packages/Python/limspath_from_cellname.py:31
- Use
is not Nonefor None checks.result != Nonecan be fooled by custom equality implementations and is not idiomatic Python.
if result != None:
Packages/Python/limspath_from_cellname.py:62
- These commented-out exit-code lines are misleading: the script currently exits with 0 by default, so the note about “Not return zero” is inconsistent with the actual behavior. Please either implement the intended non-zero behavior or remove the stale comment.
# @todo Not return zero here due to WM bug #8570
# sys.exit(0)
Packages/MIES/MIES_Python.ipf:8
- This new IPF file is missing the standard Doxygen file header (e.g.
/// @file//// @brief) used throughout MIES, which makes generated documentation incomplete.
This issue also appears in the following locations of the same file:
- line 21
- line 29
#ifdef AUTOMATED_TESTING
#pragma ModuleName = MIES_PY
#endif // AUTOMATED_TESTING
Function/S PY_GetPackageFolder(string packageName)
Packages/MIES/MIES_AnalysisBrowser.ipf:3287
AB_GatherFoldersFromLIMSis currently an empty function with no documentation or behavior. If it is meant as a placeholder, add a clear TODO and mark the parameter unused; otherwise, remove it until implemented to avoid dead code.
Function AB_GatherFoldersFromLIMS(WAVE/T cellIDs)
End
tools/nwb-read-tests/nwbv2-read-test.py:35
- Using
check=Truehere raisesCalledProcessErrorand bypasses the script’s previous error-path that printed validator output. That makes failures harder to diagnose in CI. Consider returning a non-zero code while still emitting the tool output to stderr on failure.
comp = run(
["pynwb-validate", path],
stdout=PIPE,
stderr=STDOUT,
text=True,
Result: Add button to open a generic modal window where users can paste files, folders, cellids
Result: Yes don't hardcode them. Store in packages settings? Somehow encrypted or XXX?
Close #2591