migrate limits in v2.37#63
Conversation
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a new V2.37 limits migration feature to the admin-tools repository. It introduces a new endpoint constant, a helper function for issuing HTTP PUT requests to that endpoint, and a standalone migration orchestration script that iterates through all networks and variants with environment checks, dry-run support, and error handling. ChangesV2.37 Limits Migration
🚥 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/functions/networks/networks.py`:
- Around line 33-35: The migrate_v237_limits function currently issues
requests.put without a timeout which can hang the migration loop; modify
migrate_v237_limits to pass an explicit timeout to requests.put (e.g.,
timeout=10 or a small configurable constant) so the call fails fast, and keep
the existing response.raise_for_status() behavior; reference the requests.put
call and constant.MIGRATE_V237_LIMITS when making this change and optionally
catch requests.exceptions.Timeout around the call if you want to log or retry
separately.
In `@scripts/migrate_v237_limits.py`:
- Line 58: The migration banner prints the wrong version; update the print call
in migrate_v237_limits.py that currently uses "V2.11 limits migration (dry-run="
+ str(dry_run) + ")" so it reflects the correct migration version for this
script (e.g., "V2.37 limits migration (dry-run=" + str(dry_run) + ")"), keeping
the dry_run variable interpolation unchanged to avoid altering behavior.
- Line 42: The preflight check uses sys.exit() which returns zero on failure;
update the failure path where
check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME) is False to exit
with a non-zero code (e.g., sys.exit(1)) and optionally emit an error message
before exiting so CI/batch runners treat the failure as an error; modify the
line referencing check_server_status and sys.exit() accordingly.
- Around line 24-31: The current parser.add_argument for "-n/--dry-run" uses
action="store_true" so dry_run becomes False by default; change the CLI so
dry_run defaults to True and the flag explicitly enables execution (e.g., add a
new flag like "--execute" / "-x" with action="store_true" or change to
"--no-dry-run" with action="store_true" and then set dry_run = not
args.execute/args.no_dry_run), update the dry_run assignment accordingly, and
update the help text on parser.add_argument and the description to state that
the script performs migration PUTs (not deletions) and that dry-run is the
default unless the execute/no-dry-run flag is provided.
- Around line 68-74: The except block in migrate_v237_limits() is too broad and
swallows programming errors; change the handler to only catch
requests.exceptions.RequestException (e.g., replace "except Exception as e:"
with "except requests.exceptions.RequestException as e:") so that network/HTTP
failures are logged for network (network['uuid']) and variant (variant['num'])
as currently done, including the response body when available, while allowing
non-request exceptions (KeyError, TypeError, etc.) to propagate and fail fast.
🪄 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: 3ecb39eb-20ba-48b8-8fc8-3517cb497ff1
📒 Files selected for processing (3)
scripts/constant.pyscripts/functions/networks/networks.pyscripts/migrate_v237_limits.py
| def migrate_v237_limits(network_id, variant_num): | ||
| response = requests.put(constant.MIGRATE_V237_LIMITS.format(networkId=network_id, variantNum=variant_num)) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the networks.py file
find . -path "*scripts/functions/networks/networks.py" -type fRepository: gridsuite/admin-tools
Length of output: 106
🏁 Script executed:
# Show the content around lines 33-35
if [ -f "scripts/functions/networks/networks.py" ]; then
cat -n "scripts/functions/networks/networks.py" | sed -n '25,45p'
fiRepository: gridsuite/admin-tools
Length of output: 697
🏁 Script executed:
# Search for where migrate_v237_limits is called
rg "migrate_v237_limits" -n -B 2 -A 2Repository: gridsuite/admin-tools
Length of output: 1293
🏁 Script executed:
# Check for timeout patterns in similar requests in the codebase
rg "requests\.(put|post|get|patch)" -A 2 | head -100Repository: gridsuite/admin-tools
Length of output: 7742
🏁 Script executed:
# Look for timeout constants defined
rg "TIMEOUT|timeout" scripts/functions/networks/ -nRepository: gridsuite/admin-tools
Length of output: 47
🏁 Script executed:
# Examine the migrate_v237_limits.py file to see the full loop context
cat -n "scripts/migrate_v237_limits.py" | sed -n '60,75p'Repository: gridsuite/admin-tools
Length of output: 1082
🏁 Script executed:
# Check for any timeout configurations or constants
rg "timeout" -i --max-count 20Repository: gridsuite/admin-tools
Length of output: 3227
🏁 Script executed:
# Check similar migration functions to see if any have timeouts
cat -n "scripts/functions/networks/networks.py" | head -40Repository: gridsuite/admin-tools
Length of output: 1682
Add an explicit timeout to prevent indefinite blocking in the migration loop.
This function is called within a nested loop over all networks and variants. Without a timeout, a single hung HTTP request will stall the entire migration indefinitely. The fix is straightforward.
Suggested fix
def migrate_v237_limits(network_id, variant_num):
- response = requests.put(constant.MIGRATE_V237_LIMITS.format(networkId=network_id, variantNum=variant_num))
+ response = requests.put(
+ constant.MIGRATE_V237_LIMITS.format(networkId=network_id, variantNum=variant_num),
+ timeout=30,
+ )
response.raise_for_status()🧰 Tools
🪛 Ruff (0.15.13)
[error] 34-34: Probable use of requests call without timeout
(S113)
🤖 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/networks/networks.py` around lines 33 - 35, The
migrate_v237_limits function currently issues requests.put without a timeout
which can hang the migration loop; modify migrate_v237_limits to pass an
explicit timeout to requests.put (e.g., timeout=10 or a small configurable
constant) so the call fails fast, and keep the existing
response.raise_for_status() behavior; reference the requests.put call and
constant.MIGRATE_V237_LIMITS when making this change and optionally catch
requests.exceptions.Timeout around the call if you want to log or retry
separately.
| parser = argparse.ArgumentParser(description='Send requests to the gridsuite services to migrate V2.37 limits', ) | ||
|
|
||
| parser.add_argument("-n", "--dry-run", help="test mode (default) will not execute any deletion request", | ||
| action="store_true") | ||
|
|
||
|
|
||
| args = parser.parse_args() | ||
| dry_run = args.dry_run |
There was a problem hiding this comment.
Make dry-run the real default before this runs against all networks.
With action="store_true", running the script with no flags sets dry_run to False, so it will execute the migration even though the help text says dry-run is the default. The help text also mentions deletion requests, but this script issues migration PUTs.
Suggested fix
parser = argparse.ArgumentParser(description='Send requests to the gridsuite services to migrate V2.37 limits', )
-parser.add_argument("-n", "--dry-run", help="test mode (default) will not execute any deletion request",
- action="store_true")
+parser.add_argument(
+ "-n",
+ "--dry-run",
+ dest="dry_run",
+ action="store_true",
+ default=True,
+ help="test mode (default) will not execute any migration request",
+)
+parser.add_argument(
+ "--execute",
+ dest="dry_run",
+ action="store_false",
+ help="apply the migration requests",
+)
args = parser.parse_args()
dry_run = args.dry_run🤖 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/migrate_v237_limits.py` around lines 24 - 31, The current
parser.add_argument for "-n/--dry-run" uses action="store_true" so dry_run
becomes False by default; change the CLI so dry_run defaults to True and the
flag explicitly enables execution (e.g., add a new flag like "--execute" / "-x"
with action="store_true" or change to "--no-dry-run" with action="store_true"
and then set dry_run = not args.execute/args.no_dry_run), update the dry_run
assignment accordingly, and update the help text on parser.add_argument and the
description to state that the script performs migration PUTs (not deletions) and
that dry-run is the default unless the execute/no-dry-run flag is provided.
| print("\n") | ||
|
|
||
| # Check network-store-server | ||
| if not check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME): sys.exit() |
There was a problem hiding this comment.
Exit non-zero when the preflight check fails.
A bare sys.exit() returns success here, so CI or batch runners can treat “network-store-server unavailable” as a successful migration run.
Suggested fix
-if not check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME): sys.exit()
+if not check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME):
+ sys.exit(1)📝 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.
| if not check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME): sys.exit() | |
| if not check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME): | |
| sys.exit(1) |
🧰 Tools
🪛 Ruff (0.15.13)
[error] 42-42: Multiple statements on one line (colon)
(E701)
🤖 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/migrate_v237_limits.py` at line 42, The preflight check uses
sys.exit() which returns zero on failure; update the failure path where
check_server_status(constant.NETWORK_STORE_SERVER_HOSTNAME) is False to exit
with a non-zero code (e.g., sys.exit(1)) and optionally emit an error message
before exiting so CI/batch runners treat the failure as an error; modify the
line referencing check_server_status and sys.exit() accordingly.
| print("For a total of " + str(len(networks)) + " networks") | ||
| print("---------------------------------------------------------") | ||
|
|
||
| print("V2.11 limits migration (dry-run=" + str(dry_run) + ") in processing...") |
There was a problem hiding this comment.
Fix the migration banner version.
This still logs V2.11 limits migration, which is easy to confuse with the older migration during operations.
Suggested fix
-print("V2.11 limits migration (dry-run=" + str(dry_run) + ") in processing...")
+print("V2.37 limits migration (dry-run=" + str(dry_run) + ") in processing...")📝 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.
| print("V2.11 limits migration (dry-run=" + str(dry_run) + ") in processing...") | |
| print("V2.37 limits migration (dry-run=" + str(dry_run) + ") in processing...") |
🤖 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/migrate_v237_limits.py` at line 58, The migration banner prints the
wrong version; update the print call in migrate_v237_limits.py that currently
uses "V2.11 limits migration (dry-run=" + str(dry_run) + ")" so it reflects the
correct migration version for this script (e.g., "V2.37 limits migration
(dry-run=" + str(dry_run) + ")"), keeping the dry_run variable interpolation
unchanged to avoid altering behavior.
| except Exception as e: | ||
| failCount += 1 | ||
| # print only str(e) instead of the full traceback because we call this method from a simple for loop script | ||
| tqdm.write("network " + network['uuid'] + ", variantNum " + str(variant['num']) + " => migration failed: "+ str(e)) | ||
| if isinstance(e, requests.exceptions.RequestException) and 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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/migrate_v237_limits.py | head -100Repository: gridsuite/admin-tools
Length of output: 3740
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "def migrate_v237_limits"Repository: gridsuite/admin-tools
Length of output: 106
🏁 Script executed:
cat -n ./scripts/functions/networks/networks.py | head -200Repository: gridsuite/admin-tools
Length of output: 1682
Narrow exception handling to requests failures only.
The migrate_v237_limits() function only raises requests.exceptions.RequestException and its subclasses (from requests.put() and response.raise_for_status()). Catching all Exception masks programming bugs (e.g., KeyError from malformed API data) that should fail fast rather than continue with partial migrations.
Suggested fix
- except Exception as e:
+ except requests.exceptions.RequestException as e:
failCount += 1
# print only str(e) instead of the full traceback because we call this method from a simple for loop script
tqdm.write("network " + network['uuid'] + ", variantNum " + str(variant['num']) + " => migration failed: "+ 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🧰 Tools
🪛 Ruff (0.15.13)
[warning] 68-68: Do not catch blind exception: Exception
(BLE001)
🤖 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/migrate_v237_limits.py` around lines 68 - 74, The except block in
migrate_v237_limits() is too broad and swallows programming errors; change the
handler to only catch requests.exceptions.RequestException (e.g., replace
"except Exception as e:" with "except requests.exceptions.RequestException as
e:") so that network/HTTP failures are logged for network (network['uuid']) and
variant (variant['num']) as currently done, including the response body when
available, while allowing non-request exceptions (KeyError, TypeError, etc.) to
propagate and fail fast.
|
1 participant
PR Summary