Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

automatically appending local rucio path #1182

Merged
merged 13 commits into from Jun 10, 2023
Merged

automatically appending local rucio path #1182

merged 13 commits into from Jun 10, 2023

Conversation

FaroutYLq
Copy link
Contributor

What does the code in this PR do / what does it improve?

Liberate users from manually adding midway rucio RSE by specifying

st_midway = cutax.xenonnt_online(_rucio_local_path='/project/lgrandi/rucio', include_rucio_local = True)

Can you briefly describe how it works?

Introduced a new parameter in xenonnt_online called _auto_append_rucio_local. If True (by default it is True), it will make include_rucio_local = True and append the midway rucio path in _rucio_local_path.

Can you give a minimal working example (or illustrate with a figure)?

After modification, st = cutax.xenonnt_online() will have access to data in midway rucio if you are not on dali compute nodes.

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
straxen/contexts.py Outdated Show resolved Hide resolved
@FaroutYLq
Copy link
Contributor Author

This is just a dirty workaround. The proper way to do it will require decoupling hardcoding in straxen.RunDB. For example: here.

@FaroutYLq
Copy link
Contributor Author

Meanwhile, we should probably also refactor _rucio_local_path. It has been marked "Only use for testing!" for a while...

@coveralls
Copy link

coveralls commented May 15, 2023

Coverage Status

coverage: 93.459% (+0.04%) from 93.417% when pulling 00d4048 on which_rucio into f39d8ea on master.

@FaroutYLq FaroutYLq marked this pull request as ready for review May 16, 2023 00:15
@FaroutYLq FaroutYLq changed the title automatically appending local/remote rucio path automatically appending local rucio path May 16, 2023
@dachengx dachengx self-requested a review May 16, 2023 23:57
@dachengx
Copy link
Collaborator

Whether we want to merge it like this depends on whether we want to postpone the complete decoupling of dali in straxen's hardcode.

Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FaroutYLq .

st = straxen.contexts.xenonnt_online()

On dali computing node, storages are:

[straxen.storage.rundb.RunDB, readonly: True,
 strax.storage.files.DataDirectory, readonly: True, path: /dali/lgrandi/xenonnt/raw, take_only: ('raw_records', 'raw_records_he', 'raw_records_aqmon', 'raw_records_nv', 'raw_records_aqmon_nv', 'raw_records_aux_mv', 'raw_records_mv'),
 strax.storage.files.DataDirectory, readonly: True, path: /dali/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, readonly: True, path: /project2/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, readonly: True, path: /project/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, path: ./strax_data]

On xenon1t computing node, storages are:

[straxen.storage.rundb.RunDB, readonly: True,
 strax.storage.files.DataDirectory, readonly: True, path: /dali/lgrandi/xenonnt/raw, take_only: ('raw_records', 'raw_records_he', 'raw_records_aqmon', 'raw_records_nv', 'raw_records_aqmon_nv', 'raw_records_aux_mv', 'raw_records_mv'),
 strax.storage.files.DataDirectory, readonly: True, path: /dali/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, readonly: True, path: /project2/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, readonly: True, path: /project/lgrandi/xenonnt/processed,
 strax.storage.files.DataDirectory, path: ./strax_data,
 straxen.storage.rucio_local.RucioLocalFrontend, readonly: True, path: /project/lgrandi/rucio/]

Looks good to me.

@FaroutYLq
Copy link
Contributor Author

FaroutYLq commented May 18, 2023

Since we are having tutorial for users to load data now, I feel less urgency to do this dirty workaround. Personally I suggest in straxen we do things technically better rather than quick dirty tricks like what I did. I would close it for now and let's pick up once we have more elegant solution (and time : )). Thanks @dachengx

@FaroutYLq FaroutYLq closed this May 18, 2023
@FaroutYLq
Copy link
Contributor Author

@dachengx . I discussed with @jingqiangye yesterday and we think for now the user experience might be more important than software logic elegancy. Let's reconsider merging this for the coming environment release.

@FaroutYLq FaroutYLq reopened this Jun 3, 2023
@dachengx
Copy link
Collaborator

@dachengx . I discussed with @jingqiangye yesterday and we think for now the user experience might be more important than software logic elegancy. Let's reconsider merging this for the coming environment release.

No objection. Just keep in mind that this will anyway take effect after the new release of nT base_environment.

@FaroutYLq
Copy link
Contributor Author

FaroutYLq commented Jun 10, 2023 via email

@dachengx dachengx merged commit 9638896 into master Jun 10, 2023
7 checks passed
@dachengx dachengx deleted the which_rucio branch June 10, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants