Add invalidate study script#60
Open
Meklo wants to merge 9 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete study invalidation workflow. It adds two new URL constants for API endpoints, a low-level function to delete a study via HTTP, and a new CLI script that orchestrates fetching and batch-invalidating unmodified studies with progress reporting, error handling, and dry-run support. ChangesStudy Invalidation Workflow
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 2 commits
April 15, 2026 10:57
flomillot
reviewed
Apr 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/functions/studies/studies.py`:
- Around line 40-41: The unmount_study function uses requests.delete without a
timeout which can hang; update the unmount_study(study_uuid) function to pass a
timeout argument to requests.delete (e.g., timeout=10 or preferably a shared
config constant like REQUEST_TIMEOUT) when calling
constant.UNMOUNT_STUDY.format(studyUuid=study_uuid), and handle or let
exceptions propagate as before so the call fails fast on network issues.
In `@scripts/unmount_unmodified_studies.py`:
- Line 52: The print statement that uses an f-string without placeholders (the
print call that outputs "Studies to process:") should be changed to a plain
string literal by removing the unnecessary f prefix on that print invocation;
update the print in scripts/unmount_unmodified_studies.py accordingly so linters
no longer flag Ruff F541/Sonar, then run the linter/formatters to confirm the
warning is resolved.
- Around line 79-101: The CLI parsing and invocation using sys.argv and sys.exit
should be moved into a main guard to avoid executing on import; wrap the block
that reads duration_arg, dry_run_arg, batch_size_arg and the final call to
unmount_unmodified_studies(duration_arg, dry_run=..., batch_size=...) inside if
__name__ == "__main__": so importing this module won't access sys.argv or call
sys.exit (this preserves safe reuse of functions like unmount_unmodified_studies
and get_unmodified_studies).
- Around line 33-35: The requests.get call inside get_unmodified_studies should
include a timeout to avoid hanging; modify the call to
requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, params={"elementType":
"STUDY", "duration": duration}, timeout=30) so the HTTP request to
constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS has a 30-second timeout and still
calls response.raise_for_status() afterwards.
- Around line 66-74: The loop calling unmount_study(study_uuid) can raise
network exceptions and abort the batch; wrap the call (and subsequent
status_code handling) in a try-except that catches requests.RequestException
(and optionally Exception for safety), on exception print a FAILED line
including the study_uuid and exception message, increment failure_count, and
continue to the next study so the summary still prints; update the block around
unmount_study, result, success_count and failure_count accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90517f3d-550f-4b4f-958f-ee7fc7d71bba
📒 Files selected for processing (3)
scripts/constant.pyscripts/functions/studies/studies.pyscripts/unmount_unmodified_studies.py
Comment on lines
+40
to
+41
| def unmount_study(study_uuid): | ||
| return requests.delete(constant.UNMOUNT_STUDY.format(studyUuid = study_uuid)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "studies.py" -type fRepository: gridsuite/admin-tools
Length of output: 103
🏁 Script executed:
cat -n ./scripts/functions/studies/studies.pyRepository: gridsuite/admin-tools
Length of output: 2039
🏁 Script executed:
rg "requests\.(get|delete|post|put|patch)" ./scripts --context 2 | head -80Repository: gridsuite/admin-tools
Length of output: 6557
🏁 Script executed:
rg "timeout" ./scripts --type pyRepository: gridsuite/admin-tools
Length of output: 47
🏁 Script executed:
rg "requests\.(get|delete|post|put)" ./scripts/functions/studies/studies.py -A 1Repository: gridsuite/admin-tools
Length of output: 546
🏁 Script executed:
rg "raise_for_status|timeout" ./scripts --type py -B 2 -A 2Repository: gridsuite/admin-tools
Length of output: 7941
🏁 Script executed:
rg "unmount_study" ./scripts --type py -B 2 -A 2Repository: gridsuite/admin-tools
Length of output: 1156
Add a request timeout for the unmount call.
The requests.delete() call on line 41 lacks a timeout parameter, which can block indefinitely on connectivity issues.
Proposed fix
-def unmount_study(study_uuid):
- return requests.delete(constant.UNMOUNT_STUDY.format(studyUuid = study_uuid))
+def unmount_study(study_uuid, timeout=30):
+ return requests.delete(
+ constant.UNMOUNT_STUDY.format(studyUuid=study_uuid),
+ timeout=timeout,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def unmount_study(study_uuid): | |
| return requests.delete(constant.UNMOUNT_STUDY.format(studyUuid = study_uuid)) | |
| def unmount_study(study_uuid, timeout=30): | |
| return requests.delete( | |
| constant.UNMOUNT_STUDY.format(studyUuid=study_uuid), | |
| timeout=timeout, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 41-41: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/functions/studies/studies.py` around lines 40 - 41, The unmount_study
function uses requests.delete without a timeout which can hang; update the
unmount_study(study_uuid) function to pass a timeout argument to requests.delete
(e.g., timeout=10 or preferably a shared config constant like REQUEST_TIMEOUT)
when calling constant.UNMOUNT_STUDY.format(studyUuid=study_uuid), and handle or
let exceptions propagate as before so the call fails fast on network issues.
Comment on lines
+33
to
+35
| def get_unmodified_studies(duration): | ||
| response = requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, params={"elementType": "STUDY", "duration": duration}) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the file
find . -name "unmount_unmodified_studies.py" -type fRepository: gridsuite/admin-tools
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Search for requests.get() calls in the codebase to understand the pattern
rg "requests\.get\(" --no-heading -B 2 -A 2Repository: gridsuite/admin-tools
Length of output: 21172
🏁 Script executed:
#!/bin/bash
# Check if there's a constants or config file that might set default timeouts
fd -i "constant" --type f -x head -20 {}Repository: gridsuite/admin-tools
Length of output: 697
🏁 Script executed:
#!/bin/bash
cat -n scripts/unmount_unmodified_studies.pyRepository: gridsuite/admin-tools
Length of output: 4391
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern of timeout usage elsewhere in the codebase
rg "timeout\s*=" --type py -C 2Repository: gridsuite/admin-tools
Length of output: 47
Add timeout to HTTP request for fetching unmodified studies.
The requests.get() call on line 34 lacks a timeout parameter and can hang indefinitely, causing the job to stall. Add timeout=30 to the request.
Proposed fix
def get_unmodified_studies(duration):
- response = requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, params={"elementType": "STUDY", "duration": duration})
+ response = requests.get(
+ constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS,
+ params={"elementType": "STUDY", "duration": duration},
+ timeout=30,
+ )
response.raise_for_status()
return response.json()🧰 Tools
🪛 Ruff (0.15.10)
[error] 34-34: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/unmount_unmodified_studies.py` around lines 33 - 35, The requests.get
call inside get_unmodified_studies should include a timeout to avoid hanging;
modify the call to requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS,
params={"elementType": "STUDY", "duration": duration}, timeout=30) so the HTTP
request to constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS has a 30-second timeout
and still calls response.raise_for_status() afterwards.
Comment on lines
+79
to
+101
| if len(sys.argv) < 2: | ||
| print("Usage: python unmount_unmodified_studies.py <duration> [--dry-run] [--batch-size <n>]") | ||
| print("Example: python unmount_unmodified_studies.py P365D --batch-size 10 --dry-run") | ||
| sys.exit(1) | ||
|
|
||
| duration_arg = sys.argv[1] | ||
| dry_run_arg = "--dry-run" in sys.argv | ||
|
|
||
| batch_size_arg = None | ||
| if "--batch-size" in sys.argv: | ||
| batch_size_index = sys.argv.index("--batch-size") | ||
| if batch_size_index + 1 >= len(sys.argv): | ||
| print("Error: --batch-size requires a numeric value.") | ||
| sys.exit(1) | ||
| try: | ||
| batch_size_arg = int(sys.argv[batch_size_index + 1]) | ||
| if batch_size_arg <= 0: | ||
| raise ValueError | ||
| except ValueError: | ||
| print("Error: --batch-size must be a positive integer.") | ||
| sys.exit(1) | ||
|
|
||
| unmount_unmodified_studies(duration_arg, dry_run=dry_run_arg, batch_size=batch_size_arg) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "unmount_unmodified_studies.py" -type fRepository: gridsuite/admin-tools
Length of output: 104
🏁 Script executed:
# Check the file size first
wc -l scripts/unmount_unmodified_studies.pyRepository: gridsuite/admin-tools
Length of output: 106
🏁 Script executed:
# Read the complete file to see current structure
cat -n scripts/unmount_unmodified_studies.pyRepository: gridsuite/admin-tools
Length of output: 4391
🏁 Script executed:
# Check if this module is imported anywhere in the codebase
rg "from.*unmount_unmodified_studies|import.*unmount_unmodified_studies" --type pyRepository: gridsuite/admin-tools
Length of output: 47
Wrap CLI execution in if __name__ == "__main__": guard.
The current code at module level accesses sys.argv and calls sys.exit(1), which executes on import. This prevents the module from being safely imported for reusing the unmount_unmodified_studies() and get_unmodified_studies() functions.
Proposed fix
-if len(sys.argv) < 2:
- print("Usage: python unmount_unmodified_studies.py <duration> [--dry-run] [--batch-size <n>]")
- print("Example: python unmount_unmodified_studies.py P365D --batch-size 10 --dry-run")
- sys.exit(1)
-
-duration_arg = sys.argv[1]
-dry_run_arg = "--dry-run" in sys.argv
-
-batch_size_arg = None
-if "--batch-size" in sys.argv:
- batch_size_index = sys.argv.index("--batch-size")
- if batch_size_index + 1 >= len(sys.argv):
- print("Error: --batch-size requires a numeric value.")
- sys.exit(1)
- try:
- batch_size_arg = int(sys.argv[batch_size_index + 1])
- if batch_size_arg <= 0:
- raise ValueError
- except ValueError:
- print("Error: --batch-size must be a positive integer.")
- sys.exit(1)
-
-unmount_unmodified_studies(duration_arg, dry_run=dry_run_arg, batch_size=batch_size_arg)
+def main():
+ if len(sys.argv) < 2:
+ print("Usage: python unmount_unmodified_studies.py <duration> [--dry-run] [--batch-size <n>]")
+ print("Example: python unmount_unmodified_studies.py P365D --batch-size 10 --dry-run")
+ sys.exit(1)
+
+ duration_arg = sys.argv[1]
+ dry_run_arg = "--dry-run" in sys.argv
+
+ batch_size_arg = None
+ if "--batch-size" in sys.argv:
+ batch_size_index = sys.argv.index("--batch-size")
+ if batch_size_index + 1 >= len(sys.argv):
+ print("Error: --batch-size requires a numeric value.")
+ sys.exit(1)
+ try:
+ batch_size_arg = int(sys.argv[batch_size_index + 1])
+ if batch_size_arg <= 0:
+ raise ValueError
+ except ValueError:
+ print("Error: --batch-size must be a positive integer.")
+ sys.exit(1)
+
+ unmount_unmodified_studies(duration_arg, dry_run=dry_run_arg, batch_size=batch_size_arg)
+
+
+if __name__ == "__main__":
+ main()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(sys.argv) < 2: | |
| print("Usage: python unmount_unmodified_studies.py <duration> [--dry-run] [--batch-size <n>]") | |
| print("Example: python unmount_unmodified_studies.py P365D --batch-size 10 --dry-run") | |
| sys.exit(1) | |
| duration_arg = sys.argv[1] | |
| dry_run_arg = "--dry-run" in sys.argv | |
| batch_size_arg = None | |
| if "--batch-size" in sys.argv: | |
| batch_size_index = sys.argv.index("--batch-size") | |
| if batch_size_index + 1 >= len(sys.argv): | |
| print("Error: --batch-size requires a numeric value.") | |
| sys.exit(1) | |
| try: | |
| batch_size_arg = int(sys.argv[batch_size_index + 1]) | |
| if batch_size_arg <= 0: | |
| raise ValueError | |
| except ValueError: | |
| print("Error: --batch-size must be a positive integer.") | |
| sys.exit(1) | |
| unmount_unmodified_studies(duration_arg, dry_run=dry_run_arg, batch_size=batch_size_arg) | |
| def main(): | |
| if len(sys.argv) < 2: | |
| print("Usage: python unmount_unmodified_studies.py <duration> [--dry-run] [--batch-size <n>]") | |
| print("Example: python unmount_unmodified_studies.py P365D --batch-size 10 --dry-run") | |
| sys.exit(1) | |
| duration_arg = sys.argv[1] | |
| dry_run_arg = "--dry-run" in sys.argv | |
| batch_size_arg = None | |
| if "--batch-size" in sys.argv: | |
| batch_size_index = sys.argv.index("--batch-size") | |
| if batch_size_index + 1 >= len(sys.argv): | |
| print("Error: --batch-size requires a numeric value.") | |
| sys.exit(1) | |
| try: | |
| batch_size_arg = int(sys.argv[batch_size_index + 1]) | |
| if batch_size_arg <= 0: | |
| raise ValueError | |
| except ValueError: | |
| print("Error: --batch-size must be a positive integer.") | |
| sys.exit(1) | |
| unmount_unmodified_studies(duration_arg, dry_run=dry_run_arg, batch_size=batch_size_arg) | |
| if __name__ == "__main__": | |
| main() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/unmount_unmodified_studies.py` around lines 79 - 101, The CLI parsing
and invocation using sys.argv and sys.exit should be moved into a main guard to
avoid executing on import; wrap the block that reads duration_arg, dry_run_arg,
batch_size_arg and the final call to unmount_unmodified_studies(duration_arg,
dry_run=..., batch_size=...) inside if __name__ == "__main__": so importing this
module won't access sys.argv or call sys.exit (this preserves safe reuse of
functions like unmount_unmodified_studies and get_unmodified_studies).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/unmount_unmodified_studies.py (1)
61-61: Use explicit conversion flag instead of wrapping instr().Ruff RUF010: replace
{str(constant.DEV)}with{constant.DEV!s}(or simply{constant.DEV}).Proposed fix
- print(f"\nDEV={str(constant.DEV)} -> hostnames configured for a local execution (172.17.0.1:xxxx)") + print(f"\nDEV={constant.DEV!s} -> hostnames configured for a local execution (172.17.0.1:xxxx)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/unmount_unmodified_studies.py` at line 61, The f-string in the print call uses an explicit str() wrapper: replace the occurrence of str(constant.DEV) inside the f-string with either {constant.DEV!s} or simply {constant.DEV} to satisfy the RUF010 rule; update the print statement that currently reads print(f"\nDEV={str(constant.DEV)} -> hostnames configured for a local execution (172.17.0.1:xxxx)") to use the direct or !s conversion for constant.DEV.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/unmount_unmodified_studies.py`:
- Around line 38-50: Rename the internal parameter and usages from batch_size to
limit in unmount_unmodified_studies (function) and update the conditional and
slice logic to use limit, change the user-facing log from "Batch size limit
applied: processing ..." to something like "Limit applied: processing {limit}
out of {len(studies)} studies.", and update the CLI parsing variables (e.g.,
limit_arg, limit_index) so the parsed value is assigned to limit and passed into
unmount_unmodified_studies; ensure all references to batch_size (parameter,
variable, and messages) are replaced with limit and that get_unmodified_studies
usage remains unchanged.
---
Nitpick comments:
In `@scripts/unmount_unmodified_studies.py`:
- Line 61: The f-string in the print call uses an explicit str() wrapper:
replace the occurrence of str(constant.DEV) inside the f-string with either
{constant.DEV!s} or simply {constant.DEV} to satisfy the RUF010 rule; update the
print statement that currently reads print(f"\nDEV={str(constant.DEV)} ->
hostnames configured for a local execution (172.17.0.1:xxxx)") to use the direct
or !s conversion for constant.DEV.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14daab32-32b2-4b38-84c6-48b8761d9ed5
📒 Files selected for processing (1)
scripts/unmount_unmodified_studies.py
flomillot
requested changes
Apr 20, 2026
Contributor
flomillot
left a comment
flomillot
left a comment
There was a problem hiding this comment.
You didn't change the param inside the script
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/unmount_unmodified_studies.py (2)
33-36:⚠️ Potential issue | 🟠 MajorAdd a timeout to the study lookup request.
Line 34 still calls
requests.get(...)without a timeout, so the job can hang indefinitely if the directory service stalls. This was already flagged in an earlier review.🐛 Proposed fix
def get_unmodified_studies(duration): - response = requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, params={"elementType": "STUDY", "duration": duration}) + response = requests.get( + constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, + params={"elementType": "STUDY", "duration": duration}, + timeout=30, + ) response.raise_for_status() return response.json()To verify remaining timeout gaps:
#!/bin/bash rg -n -C2 'requests\.(get|post|put|patch|delete)\(' --type py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/unmount_unmodified_studies.py` around lines 33 - 36, The get_unmodified_studies function makes a requests.get call without a timeout which can hang; modify the requests.get call inside get_unmodified_studies to include a sensible timeout (e.g., timeout=10) or accept a timeout parameter and pass it through, ensure response.raise_for_status() behavior remains the same, and run the suggested ripgrep check to find other requests.* calls that also lack timeouts.
66-74:⚠️ Potential issue | 🟠 MajorHarden the per-study unmount call.
unmount_study(...)can raise arequests.RequestException, aborting the remaining studies and skipping the summary; that part was already flagged earlier. Also, the providedscripts/functions/studies/studies.py:40-42helper usesrequests.delete(...)without a timeout, so one stuck DELETE can hang the whole run.🛡️ Proposed loop fix
for study in studies: study_uuid = study["elementUuid"] - result = unmount_study(study_uuid) - if result.status_code == 200: - print(f" OK - {study_uuid}") - success_count += 1 - else: - print(f" FAILED - {study_uuid} (status: {result.status_code})") + try: + result = unmount_study(study_uuid) + if result.status_code == 200: + print(f" OK - {study_uuid}") + success_count += 1 + else: + print(f" FAILED - {study_uuid} (status: {result.status_code})") + failure_count += 1 + except requests.RequestException as exc: + print(f" FAILED - {study_uuid} (exception: {exc})") failure_count += 1Also add a timeout in
scripts/functions/studies/studies.py:def unmount_study(study_uuid): - return requests.delete(constant.UNMOUNT_STUDY.format(studyUuid = study_uuid)) + return requests.delete(constant.UNMOUNT_STUDY.format(studyUuid=study_uuid), timeout=30)To verify both pieces:
#!/bin/bash rg -n -C3 'def unmount_study|requests\.delete\(|RequestException|unmount_study\(' --type py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/unmount_unmodified_studies.py` around lines 66 - 74, Wrap each per-study call to unmount_study inside a try/except that catches requests.RequestException (or broader Exception if needed) so one failing DELETE doesn't abort the loop and always updates/prints success_count or failure_count and the final summary; specifically surround the call in the loop that updates success_count/failure_count and on exception print a FAILED line with the exception message and increment failure_count. Also add a timeout argument to the requests.delete call inside the unmount_study implementation (requests.delete(..., timeout=...)) and handle/propagate RequestException from that helper so the loop’s try/except can catch it.
🧹 Nitpick comments (1)
scripts/unmount_unmodified_studies.py (1)
60-61: Use the f-string conversion flag for the DEV log.Line 61 triggers Ruff RUF010; prefer
!sinstead of wrapping the value withstr(...).🧹 Proposed fix
if constant.DEV: - print(f"\nDEV={str(constant.DEV)} -> hostnames configured for a local execution (172.17.0.1:xxxx)") + print(f"\nDEV={constant.DEV!s} -> hostnames configured for a local execution (172.17.0.1:xxxx)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/unmount_unmodified_studies.py` around lines 60 - 61, The f-string uses str(constant.DEV) which triggers Ruff RUF010; update the print statement that references constant.DEV to use the f-string conversion flag instead (replace the str(...) call with {constant.DEV!s}) so the print line using constant.DEV outputs the same string value without calling str explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/unmount_unmodified_studies.py`:
- Around line 79-84: The usage guard incorrectly checks if len(sys.argv) < 1
which is always false; update the check in scripts/unmount_unmodified_studies.py
to if len(sys.argv) < 2 so the script prints the usage and exits when no
duration argument is provided, ensuring the subsequent access to duration_arg =
sys.argv[1] is safe; leave the rest of the argument parsing (including handling
--dry-run and --limit) intact.
---
Duplicate comments:
In `@scripts/unmount_unmodified_studies.py`:
- Around line 33-36: The get_unmodified_studies function makes a requests.get
call without a timeout which can hang; modify the requests.get call inside
get_unmodified_studies to include a sensible timeout (e.g., timeout=10) or
accept a timeout parameter and pass it through, ensure
response.raise_for_status() behavior remains the same, and run the suggested
ripgrep check to find other requests.* calls that also lack timeouts.
- Around line 66-74: Wrap each per-study call to unmount_study inside a
try/except that catches requests.RequestException (or broader Exception if
needed) so one failing DELETE doesn't abort the loop and always updates/prints
success_count or failure_count and the final summary; specifically surround the
call in the loop that updates success_count/failure_count and on exception print
a FAILED line with the exception message and increment failure_count. Also add a
timeout argument to the requests.delete call inside the unmount_study
implementation (requests.delete(..., timeout=...)) and handle/propagate
RequestException from that helper so the loop’s try/except can catch it.
---
Nitpick comments:
In `@scripts/unmount_unmodified_studies.py`:
- Around line 60-61: The f-string uses str(constant.DEV) which triggers Ruff
RUF010; update the print statement that references constant.DEV to use the
f-string conversion flag instead (replace the str(...) call with
{constant.DEV!s}) so the print line using constant.DEV outputs the same string
value without calling str explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5db71ba-d340-42b0-98a8-53d6f7f560b8
📒 Files selected for processing (1)
scripts/unmount_unmodified_studies.py
etiennehomer
requested changes
Apr 21, 2026
| print("\nDry run mode: no study will be unmounted.") | ||
| return | ||
|
|
||
| if constant.DEV: |
| failure_count = 0 | ||
| for study in studies: | ||
| study_uuid = study["elementUuid"] | ||
| result = unmount_study(study_uuid) |
Contributor
There was a problem hiding this comment.
Use a try catch and raise_for_status() as it is done for migrate_v211_limits.py or copy_cases_to_s3_storage.py
It allows to print error logs
Contributor
There was a problem hiding this comment.
We should get and print some errors when some networks linked to study are not found
| study_uuid = study["elementUuid"] | ||
| result = unmount_study(study_uuid) | ||
| if result.status_code == 200: | ||
| print(f" OK - {study_uuid}") |
Contributor
There was a problem hiding this comment.
We don't need to log success logs. It's easier to read the logs with only error logs.
And the success resumee at the end
etiennehomer
requested changes
Apr 21, 2026
| print("\nUnmounting studies...") | ||
| success_count = 0 | ||
| failure_count = 0 | ||
| for study in studies: |
Contributor
There was a problem hiding this comment.
use tqdm to follow progression
flomillot
approved these changes
May 11, 2026
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/functions/studies/studies.py (1)
40-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the invalidate DELETE call.
requests.delete(...)here has no timeout, so this call can hang indefinitely on network issues.Proposed fix
-def invalidate_study(study_uuid): - return requests.delete(constant.INVALIDATE_STUDY.format(studyUuid = study_uuid)) +def invalidate_study(study_uuid, timeout=30): + return requests.delete( + constant.INVALIDATE_STUDY.format(studyUuid=study_uuid), + timeout=timeout, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/functions/studies/studies.py` around lines 40 - 41, The invalidate_study function currently calls requests.delete without a timeout which can hang; update invalidate_study to pass a timeout parameter to requests.delete (e.g., timeout=5 or use a configured constant) so the DELETE call fails fast on network issues, e.g., requests.delete(constant.INVALIDATE_STUDY.format(studyUuid=study_uuid), timeout=<value>), and ensure any callers handle the potential Timeout or RequestException if necessary.scripts/invalidate_unmodified_studies.py (1)
34-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a timeout on the unmodified-studies fetch request.
Line 35 performs a network call without timeout, which can stall the script indefinitely.
Proposed fix
def get_unmodified_studies(duration): - response = requests.get(constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, params={"elementType": "STUDY", "duration": duration}) + response = requests.get( + constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, + params={"elementType": "STUDY", "duration": duration}, + timeout=30, + ) response.raise_for_status() return response.json()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/invalidate_unmodified_studies.py` around lines 34 - 37, get_unmodified_studies calls requests.get without a timeout which can hang; update the function get_unmodified_studies to pass a sensible timeout (e.g., timeout=10 or a configurable constant) into requests.get when calling constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, and ensure response.raise_for_status() remains after the timed request so failures still surface.
🧹 Nitpick comments (1)
scripts/invalidate_unmodified_studies.py (1)
83-105: ⚡ Quick winGuard CLI execution behind a main entrypoint.
Running argument parsing at import-time makes this module hard to reuse and test safely.
Proposed refactor
-if len(sys.argv) < 2: - print("Usage: python invalidate_unmodified_studies.py <duration> [--dry-run] [--limit <n>]") - print("Example: python invalidate_unmodified_studies.py P365D --limit 10 --dry-run") - sys.exit(1) - -duration_arg = sys.argv[1] -dry_run_arg = "--dry-run" in sys.argv - -limit_arg = None -if "--limit" in sys.argv: - limit_index = sys.argv.index("--limit") - if limit_index + 1 >= len(sys.argv): - print("Error: --limit requires a numeric value.") - sys.exit(1) - try: - limit_arg = int(sys.argv[limit_index + 1]) - if limit_arg <= 0: - raise ValueError - except ValueError: - print("Error: --limit must be a positive integer.") - sys.exit(1) - -invalidate_unmodified_studies(duration_arg, dry_run=dry_run_arg, limit=limit_arg) +if __name__ == "__main__": + if len(sys.argv) < 2: + print("Usage: python invalidate_unmodified_studies.py <duration> [--dry-run] [--limit <n>]") + print("Example: python invalidate_unmodified_studies.py P365D --limit 10 --dry-run") + sys.exit(1) + + duration_arg = sys.argv[1] + dry_run_arg = "--dry-run" in sys.argv + + limit_arg = None + if "--limit" in sys.argv: + limit_index = sys.argv.index("--limit") + if limit_index + 1 >= len(sys.argv): + print("Error: --limit requires a numeric value.") + sys.exit(1) + try: + limit_arg = int(sys.argv[limit_index + 1]) + if limit_arg <= 0: + raise ValueError + except ValueError: + print("Error: --limit must be a positive integer.") + sys.exit(1) + + invalidate_unmodified_studies(duration_arg, dry_run=dry_run_arg, limit=limit_arg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/invalidate_unmodified_studies.py` around lines 83 - 105, The script currently parses sys.argv and calls invalidate_unmodified_studies at import time; move that CLI logic into a guarded main function to avoid side-effects on import: create a main() that contains the argument parsing (handling duration_arg, dry_run_arg, limit_arg) and the call to invalidate_unmodified_studies(duration_arg, dry_run=dry_run_arg, limit=limit_arg), then add the standard if __name__ == "__main__": main() guard so importing this module no longer executes the CLI logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/invalidate_unmodified_studies.py`:
- Around line 68-76: The except block can reference study_uuid before it's
assigned if study["elementUuid"] throws; ensure study_uuid is defined before the
try by extracting it with a safe accessor (e.g., use study.get("elementUuid",
"<unknown>") or assign study_uuid = study.get("elementUuid") above the try),
then wrap only the network call and result.raise_for_status() in the try; keep
references to invalidate_study and result.raise_for_status() inside the try so
exceptions from those are still caught but the except handler can safely log
study_uuid.
---
Duplicate comments:
In `@scripts/functions/studies/studies.py`:
- Around line 40-41: The invalidate_study function currently calls
requests.delete without a timeout which can hang; update invalidate_study to
pass a timeout parameter to requests.delete (e.g., timeout=5 or use a configured
constant) so the DELETE call fails fast on network issues, e.g.,
requests.delete(constant.INVALIDATE_STUDY.format(studyUuid=study_uuid),
timeout=<value>), and ensure any callers handle the potential Timeout or
RequestException if necessary.
In `@scripts/invalidate_unmodified_studies.py`:
- Around line 34-37: get_unmodified_studies calls requests.get without a timeout
which can hang; update the function get_unmodified_studies to pass a sensible
timeout (e.g., timeout=10 or a configurable constant) into requests.get when
calling constant.GET_UNMODIFIED_DIRECTORY_ELEMENTS, and ensure
response.raise_for_status() remains after the timed request so failures still
surface.
---
Nitpick comments:
In `@scripts/invalidate_unmodified_studies.py`:
- Around line 83-105: The script currently parses sys.argv and calls
invalidate_unmodified_studies at import time; move that CLI logic into a guarded
main function to avoid side-effects on import: create a main() that contains the
argument parsing (handling duration_arg, dry_run_arg, limit_arg) and the call to
invalidate_unmodified_studies(duration_arg, dry_run=dry_run_arg,
limit=limit_arg), then add the standard if __name__ == "__main__": main() guard
so importing this module no longer executes the CLI logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4b43c80-cebd-4530-bec1-a8133ff0516d
📒 Files selected for processing (3)
scripts/constant.pyscripts/functions/studies/studies.pyscripts/invalidate_unmodified_studies.py
✅ Files skipped from review due to trivial changes (1)
- scripts/constant.py
Comment on lines
+68
to
+76
| try: | ||
| study_uuid = study["elementUuid"] | ||
| result = invalidate_study(study_uuid) | ||
| result.raise_for_status() | ||
| success_count += 1 | ||
| except Exception as e: | ||
| failure_count += 1 | ||
| tqdm.write(f" FAILED - {study_uuid} (error: {str(e)})") | ||
| if isinstance(e, requests.exceptions.RequestException) and e.response is not None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/invalidate_unmodified_studies.py | sed -n '60,85p'Repository: gridsuite/admin-tools
Length of output: 1312
Fix unsafe exception handling: study_uuid undefined if line 69 fails.
If line 69 raises an exception (e.g., missing "elementUuid" key), study_uuid is never assigned but line 75 references it in the except block, causing a NameError that crashes the script. Move the assignment outside the try block or use .get() with a default value to ensure study_uuid is always defined before the except handler executes.
Proposed fix
- for study in tqdm(studies):
- try:
- study_uuid = study["elementUuid"]
+ for study in tqdm(studies):
+ study_uuid = study.get("elementUuid", "<missing-elementUuid>")
+ try:
+ if study_uuid == "<missing-elementUuid>":
+ raise KeyError("elementUuid")
result = invalidate_study(study_uuid)
result.raise_for_status()
success_count += 1
- except Exception as e:
+ except requests.exceptions.RequestException as e:
failure_count += 1
tqdm.write(f" FAILED - {study_uuid} (error: {str(e)})")
- if isinstance(e, requests.exceptions.RequestException) and e.response is not None:
+ if e.response is not None:
tqdm.write("Response body: " + repr(e.response.text)) # repr for cheap escaping
tqdm.write("") # emtpy newline between errors for legibility
+ except KeyError as e:
+ failure_count += 1
+ tqdm.write(f" FAILED - {study_uuid} (missing field: {str(e)})")
+ tqdm.write("")🧰 Tools
🪛 Ruff (0.15.12)
[warning] 73-73: Do not catch blind exception: Exception
(BLE001)
[warning] 75-75: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/invalidate_unmodified_studies.py` around lines 68 - 76, The except
block can reference study_uuid before it's assigned if study["elementUuid"]
throws; ensure study_uuid is defined before the try by extracting it with a safe
accessor (e.g., use study.get("elementUuid", "<unknown>") or assign study_uuid =
study.get("elementUuid") above the try), then wrap only the network call and
result.raise_for_status() in the try; keep references to invalidate_study and
result.raise_for_status() inside the try so exceptions from those are still
caught but the except handler can safely log study_uuid.
etiennehomer
approved these changes
May 13, 2026
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
3 participants
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.
PR Summary