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

Integrate use of dataregistry into gcr-catalogs #638

Merged
merged 16 commits into from
Aug 26, 2024

Conversation

JoanneBogart
Copy link
Contributor

If the dataregistry module is available in the user's environment, they can choose to use it to find and load catalogs instead of the traditional config file mechanism.

fix #637

@JoanneBogart JoanneBogart marked this pull request as draft August 13, 2024 00:34
@JoanneBogart JoanneBogart marked this pull request as ready for review August 19, 2024 17:50
@JoanneBogart JoanneBogart requested a review from yymao August 19, 2024 19:54
Copy link
Member

@yymao yymao left a comment

Choose a reason for hiding this comment

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

Thanks for preparing this @JoanneBogart. I think the proposed change makes sense, and the high-level implementation (such as creating base_config) is very reasonable, so I am approving this PR. However, I have not read the code line-by-line (especially the new dr_register.py) and have not tested this PR either. If a detailed review is needed, I can help but it will need to wait until I have more time to attend to it.

@JoanneBogart
Copy link
Contributor Author

Thank you, @yymao !
One change I made after the discussion some weeks ago was to automatically activate the cfg-file mode of access in case the dataregistry code is not available. This makes it possible to at least test that, in this case, everything behaves as expected for existing applications. If you have code lying around which exercises anything beyond the CI tests and which involves minimal effort on your part, please run it and let me know if you see any glitches.
However users who can successfully import dataregistry will have to make a call to set_config_source before they can do anything else.

@yymao
Copy link
Member

yymao commented Aug 23, 2024

I don't have other tests beyond the existing CI tests.

I wonder if it's useful to add a CI test to check the functionality of dr_register.py?

@JoanneBogart
Copy link
Contributor Author

JoanneBogart commented Aug 25, 2024

I wonder if it's useful to add a CI test to check the functionality of dr_register.py?

It would be useful but would take some work since the CI workflow has no access to NERSC resources, including the data registry Postgres database. We would have to create a database as part of the workflow and add entries. This kind of thing is done for the dataregistry CI.
I have a test script which I used to debug after seeding a Postgres database at NERSC with some entries.
dataregistry does also support sqlite to some degree. I might be able to use that for a more self-contained test. This is something I should look into after we have made a release of the dataregistry with all the support needed so that CI can easily install it.

@JoanneBogart JoanneBogart merged commit 8b94a8b into master Aug 26, 2024
3 checks passed
@JoanneBogart JoanneBogart deleted the u/jrbogart/integrate_dataregistry branch August 26, 2024 18:21
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.

Make use of dataregistry when present and requested
2 participants