-
Notifications
You must be signed in to change notification settings - Fork 967
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
Adding object store plugin for Rucio #17156
Conversation
Struggling with dependencies for Any ideas on what can be done here? |
@SergeyYakubov cool stuff. Do you think it is possible to build this dependency in theory for older Python version? We do build for a few exceptions our own wheels. https://github.com/galaxyproject/wheelforge |
My alternative advice would just be to assert at runtime that this object store only works with Python 3.9+. If that is your target platform - I don't think supporting older Pythons is important for you to do. If someone else comes along and wants to use it on an older Python we can resolve that issue then - but I think it is totally fine for newer features to only support newer Pythons. Bring in all the dependencies conditionally (ala https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/jobs/runners/util/pykube_util.py#L7) and then throw an exception at runtime if they are not available (ala https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/objectstore/pithos.py#L109). |
Ok, I think we should be good now. I do not see integration tests for Python 3.11, only unit tests, so my tests for Rucio object store are being skipped. Can we add integration tests for 3.11 (or anything above 3.8) |
@SergeyYakubov can you please rebase your PR. I hope we can than see a few more tests to pass. |
lib/galaxy/dependencies/__init__.py
Outdated
return sys.version_info >= (3, 19) | ||
|
||
|
||
# return "rucio" in self.object_stores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return sys.version_info >= (3, 19) | |
# return "rucio" in self.object_stores | |
return sys.version_info >= (3, 9) and "rucio" in self.object_stores |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though maybe skip the version_info check, if the dependency is not available we get a better error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we skip version_info check, the GitHub integration tests will be failing, unless there is a better way to disable them for Python <3.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check what python version you're in setUpClass
of BaseRucioObjectStoreIntegrationTestCase and raise unittest.SkipTest("rucio requires python >=3.9")
"code.ornl.gov:4567/ndip/public-docker/rucio:1.29.8", | ||
] | ||
subprocess.check_call(rucio_start_args) | ||
time.sleep(20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to check that the instance is up ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just some minor stuff to fix here.
a4127ca
to
3dd7a70
Compare
Oh, actually, that was not the problem (was quite some time ago, I already forgot :)). The problem was that we configure the Rucio object store during the tests, i.e. after Galaxy was started. So, "rucio" in self.object_stores is False and it does not install Rucio clients and the tests fail. We might want to unconditionally install them if the Python version is new enough. The same problem should be with irods, but it is also present in dev dependencies, so it works. |
@SergeyYakubov I do not know why. But I would like to use the opportunity to point you to https://github.com/galaxyproject/wheelforge . With this repo, we can create wheels if they are missing in pypi and publish them at https://wheels.galaxyproject.org. Maybe that helps? |
Ok, I know what the problem is. In GitHub workflows And Rucio does not expect that during setup, so we have errors. Was there a reason for having a folder with spaces? Maybe rename it for the sake of the future generations of developers' mental health :). Although this is a valid name, and with that, we test whether it works. I can also disable the tests (add |
To find this kind of issues before they reach the end users/admins :) |
created an issue rucio/rucio#6499 |
In the meantime building a wheel like @bgruening suggested would sidestep this: https://gist.github.com/mvdbeek/91a9ab31b19652dc1f54075d3b185d21 |
hmm, guess it makes sense that we don't need to build pure-python wheels ... it might be the easiest upstream fix though. |
yeah, I created a PR in the Rucio repo. Hope it'll be approved soon |
Ok, the only failing test is now CodeQL. The problem is in "password"/"secret" keywords. Not only in Rucio, but also in S3 and iRods. I don't think we can change them now - will break all deployments that use those stores. Or can we? Alternatively, is there a way to make an exception for CodeQL? @bgruening - what are your thoughts on it? |
This PR was merged without a "kind/" label, please correct. |
Congratulations @SergeyYakubov! |
With the merge of galaxyproject#17156 , `rucio-clients` was listed as a conditional requirement, but it was actually installed almost everywhere (the only condition was `sys.version_info >= (3, 9)`), independently of its use for an object store. This also installs `rucio-clients` as a dev dependency, otherwise the `test/integration/objectstore/test_rucio_objectstore.py` integration tests would fail. Pinned dependencies are updated in the next commit. Fix galaxyproject#17695 .
With the merge of galaxyproject#17156 , `rucio-clients` was listed as a conditional requirement, but it was actually installed almost everywhere (the only condition was `sys.version_info >= (3, 9)`), independently of its use for an object store. This also installs `rucio-clients` as a dev dependency, otherwise the `test/integration/objectstore/test_rucio_objectstore.py` integration tests would fail. Pinned dependencies are updated in the next commit. Fix galaxyproject#17695 .
Object store plugin for Rucio data management system (https://rucio.cern.ch).
It requires a Rucio cluster to be deployed and configured, which is not a trivial task. It works in our environment but might need adaptation to other environments/configurations. For tests, I created a standalone Rucio Docker image.
How to test the changes?
(Select all options that apply)
License