add swf_common_lib.rucio_utils derriverd from swf-common-lib/rucio_comms/utils.py - #42
Conversation
Test Coverage Summary |
calculate_file_checksum is almost the same impl as in rucio_comms, with chunk size parameter added
66dd2bf to
2a36373
Compare
Test Coverage Summary |
| logger.info(f"Dataset created: {scope}:{name}") | ||
| except DataIdentifierAlreadyExists: | ||
| logger.info(f"Dataset already exists: {scope}:{name}") | ||
|
|
There was a problem hiding this comment.
For an existing dataset, is it expected to refresh the lifetime? rucio_comms.DatasetManager.create_dataset() attempts to set lifetime for existing datasets here it's not handled.
For my usage to run a prompt processing workflow, this is not an issue because it's been always a new simulated run with a new dataset. Just think of the case when we needs to update an existing dataset.
There was a problem hiding this comment.
From what I understand one can not simply change a lifetime for a dataset, but can submit an extension request via https://rucio.cern.ch/documentation/html/client_api/lifetimeclient.html
Also, our datasets are supposed to never expire, and even for testbed they are still relatively long-living.
There was a problem hiding this comment.
And I found some slides on the topic:
https://indico.cern.ch/event/331406/contributions/770929/attachments/645580/888086/20141029TimDataLifeCycle.pdf
There was a problem hiding this comment.
Now I understand why you removed the set_metadata part. For an existing DID, that's questionable to extend the lifecycle.
Actually I do set lifetime=7days for prompt processing runs on the testbed. It used to be 1 day and I found inconvenient when I need to find some recent runs but disappear next day.
Then I think this PR is good to merge.
There was a problem hiding this comment.
Actually, maybe set_metadata works to extend. Seems like it just recalculates expired_at as utcnow() + lifetime and writes that into database.
There was a problem hiding this comment.
I've not actually used it so far because all prompt processing workflows are newly simulated runs. In rucio_comms, it's after DataIdentifierAlreadyExists so I think it attempts to refresh an existing DID lifetime.
Whether set_metadata is valid needs to be tested if we don't know. However I think your question is worth consideration. Should an existing DID be renewed? Is it something about the experiment policy?
There was a problem hiding this comment.
Dug out some old logs and I found this output:
2026-07-01 15:44:35,510 - INFO - rucio_comms.rucio_comms - Set lifetime to 7 days for group.daq:swf.102682.run
Looks like even for a new run, that block is executed actually.
There was a problem hiding this comment.
I don't see why we would want to have finite dataset lifetimes. In any case, this code does not consider replica policies. But I did reinstate the lifetime extension, I think it won't hurt to have it, and it matches original PanDA behavior.
|
|
||
| # Step 2: Check if DID already exists | ||
| try: | ||
| existing_did = data_obj.did_client.get_did(data_obj.rucio_scope, file_name) |
There was a problem hiding this comment.
I noticed this is a pre-existing issue that's also in rucio_comms. Can it be fixed here? This helper checks data_obj.did_client, but data agent doesn't have the attribute. Should it be rucio_did_client so there is no need to fix in data agent?
I've not encountered any issue yet with this bug because the prompt processing always registers new datasets :-) If not looking at this brach, I've never noticed this issue.
There was a problem hiding this comment.
Nice catch, I've updated in both places.
Test Coverage Summary |
Test Coverage Summary |
Test Coverage Summary |
There was a problem hiding this comment.
Pull request overview
This PR moves Rucio-related helpers into the installable swf_common_lib package (src/ layout) and removes the legacy rucio_comms module that required PYTHONPATH/RUCIO_COMMS_PATH to import reliably. This aligns the Rucio utilities with how the rest of the library is consumed by downstream projects (e.g., swf-testbed).
Changes:
- Added
src/swf_common_lib/rucio_utils.pywith standalone utility functions for scope extraction, checksums, dataset creation, and file attachment. - Removed the legacy
rucio_commspackage/modules from the repository. - Updated
pyproject.tomlmypy configuration to ignore missing imports forrucio.*.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/swf_common_lib/rucio_utils.py | New packaged Rucio utility module intended to replace rucio_comms imports. |
| rucio_comms/utils.py | Removed legacy utilities module (previous home of similar helpers). |
| rucio_comms/rucio_comms.py | Removed legacy higher-level orchestration/classes module. |
| rucio_comms/README.md | Removed README describing the legacy rucio_comms package. |
| rucio_comms/exceptions.py | Removed legacy exception definitions tied to rucio_comms. |
| rucio_comms/init.py | Removed legacy package initializer. |
| pyproject.toml | Added mypy override to ignore missing imports for rucio.*. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Test Coverage Summary |
Test Coverage Summary |
|
Merge is approved. After that, I think you need a follow up to remove the fallback to rucio_comms in swf-testbed, because you've removed rucio_comms. |
rucio_comms is not a real import from the package and relies on
RUCIO_COMMS_PATHor manualPYTHONPATHentries.The new implementations don't use classes. We only use static functions and it's okay to have most simplest forms until we know we need something special.
This will be used by BNLNPPS/swf-testbed#62