Repository navigation
Conversation
|
REQUEST FOR PRODUCTION RELEASES: This will add The following labels are available |
c3c0e1f to
7085882
Compare
sawenzel
left a comment
sawenzel
left a comment
There was a problem hiding this comment.
Thanks, this is a good step towards https://its.cern.ch/jira/browse/O2-6543. Apart from the small inline points, I would merge it as is, since nothing uses the script in production yet.
One question for the next iteration: the object is keyed by Period and Pass, but the value is estimated from a single run. Do we know that all runs of a pass are reconstructed close enough in time for one value to fit them all? If not, we could key by run, or have each job compare the shared value with its own estimate and warn when a reference object like TPC/Calib/CorrectionMapV2 resolves differently.
Next steps: publish and consume the object from o2dpg_sim_workflow_anchored.py, following MCProdInfo, with ALIEN_JDL_CCDB_CONDITION_NOT_AFTER kept as the override. The certificate and upload helpers could then move to a common module, as your FIXME already suggests.
| if check is None or "error" in check: | ||
| reason = check["error"] if check else "unknown error" | ||
| print(f"[getCCDBTimeMachineTimestamp] CCDB cross-check on {args.ccdb_check_path} skipped: {reason}", | ||
| print(f"[getCCDBTimeMachineTimestamp] CCDB cross-check on {ccdb_check_path}/{check["timestamp"]} skipped: {reason}", |
There was a problem hiding this comment.
Nested double quotes inside an f-string are a SyntaxError before Python 3.12. Could you use check['timestamp'] here and in the two prints below? Note also that check can be None, or an error dict without timestamp, in this branch.
| if missing: | ||
| parser.error("missing required value(s): " + ", ".join(missing)) | ||
|
|
||
| get_CCDBTimeMachineTS(args.production_tag, args.year, args.period, args.run, args.passname, |
There was a problem hiding this comment.
The value is no longer printed, so TS=$(getCCDBTimeMachineTimestamp.py ...) gets nothing. Could you add print(...) of the returned value here?
| user_path = 'Users/' + user[0] + '/' + user | ||
| url = f"{base_url}/browse/{user_path}/CCDBTimeMachineTS/{query}" | ||
|
|
||
| print(f"[getCCDBTimeMachineTimestamp] Checking if CCDBTimeMachineTS already exists: {url}") |
There was a problem hiding this comment.
This should go to sys.stderr like the other log lines, so that stdout only carries the value. Same for the "Estimating" print in get_CCDBTimeMachineTS.
| print(f"[getCCDBTimeMachineTimestamp] Checking if CCDBTimeMachineTS already exists: {url}") | ||
|
|
||
| response = requests.get(url, cert=(cert_path, key_path), verify=False) | ||
| if response.status_code != 404: |
There was a problem hiding this comment.
A 403 or 500 is treated as "found" here. Maybe check for response.status_code == 200?
ehellbar
changed the title
|
@sawenzel thanks for the review. I pushed two more commits which should address your suggestions. |
2 participants
No description provided.