Feature #22058
closedcwltest plugin to handle keep: URIs
Description
cwltest
is a tool for testing CWL workflows, in particular running the CWL conformance tests. It runs a workflow with known input and checks it against expected output.
cwltest
used to only do weak checking of workflow output -- it checked that the reported filenames and checksums matched.
However, a year+ ago, a contributor made that checking more robust, and so that it actually checks that files existed and have the expected size and content. However the implementation only worked with local files.
As a result, since then we've been pinned to the version of cwltest
immediately before the change.
This ticket is to make the file checks made by cwltest
access Keep, so we get the benefit of more robust checking as well as any other cwltest improvements since then.
Updated by Peter Amstutz 3 months ago
- Target version changed from Development 2024-10-09 sprint to Development 2024-10-23 sprint
Updated by Peter Amstutz 2 months ago
- Target version changed from Development 2024-10-23 sprint to Development 2024-11-20
Updated by Peter Amstutz about 1 month ago
- Status changed from New to In Progress
Updated by Peter Amstutz about 1 month ago
- Target version changed from Development 2024-11-20 to Development 2024-12-04
Updated by Peter Amstutz 30 days ago
22058-cwltest-plugin @ fcac5ad33bd215a7c7d7e15ef9f57e0ac8a02f24
This is the corresponding Arvados branch that to make use of the new file access plug-in interface implemented in cwltest
here: https://github.com/common-workflow-language/cwltest/pull/233
Updated by Brett Smith 29 days ago
Peter Amstutz wrote in #note-8:
def get_fsaccess():
api_client = arvados.api()
Please pass 'v1'
to arvados.api
to avoid spurious warning spew.
Other than that, the code looks good to me. I have questions about process.
Is there a cwltest release that includes the support we need to be able to start using that now? If so, should this branch remove/update our cwltest version pins to make that happen? Or does it need to happen in a separate branch for some logistical reason?
If it happens in this branch, is it possible to run the CWL conformance tests on Jenkins against this update to show it all working?
Thanks.
Updated by Peter Amstutz 26 days ago
Brett Smith wrote in #note-9:
Is there a cwltest release that includes the support we need to be able to start using that now? If so, should this branch remove/update our cwltest version pins to make that happen? Or does it need to happen in a separate branch for some logistical reason?
When I put this up for review on Friday, I'm not sure if Michael had made a new cwltest release yet, but that has since happened.
I have updated the pin in test_with_arvbox to set the minimum version to the new version.
The other pin is in the configuration for https://ci.arvados.org/view/Arvados%20build%20pipeline/job/run-tests-cwl-suite/
This gets arvados-cwl-runner from our packages and cwltest from pip. For cwltest to access Keep, an appropriate version of arvados-cwl-runner needs to be installed in the same environment as cwltest. We could get arvados-cwl-runner from git, or publish an appropriate dev package to PyPi, but these are not exactly ideal solutions as we end up with two different copies of arvados-cwl-runner which creates potential for confusion.
So it occurred to me, we could include cwltest in the arvados-cwl-runner package. What do you think?
If it happens in this branch, is it possible to run the CWL conformance tests on Jenkins against this update to show it all working?
22058-cwltest-plugin @ 522226b8f2c410a881bdba86cee02560accea2ba
arvados-cwl-conformance-tests: #1822
See note above about run-tests-cwl-suite.
Updated by Brett Smith 25 days ago
Peter Amstutz wrote in #note-10:
I have updated the pin in test_with_arvbox to set the minimum version to the new version.
I think it would be a nice improvement if 4729a5ffb5ae8f5365f3522a27f9c5c161fe8cc7 used git -C
instead of cd; git
.
It's a maintainability thing. cd
changes a global variable. When a shell script does a lot of cd
it gets hard to maintain because you have to think about which steps rely on which $PWD
when. Using git -C
(or env -C
or whatever) makes life easier long-term.
This gets arvados-cwl-runner from our packages and cwltest from pip. For cwltest to access Keep, an appropriate version of arvados-cwl-runner needs to be installed in the same environment as cwltest. We could get arvados-cwl-runner from git, or publish an appropriate dev package to PyPi, but these are not exactly ideal solutions as we end up with two different copies of arvados-cwl-runner which creates potential for confusion.
I think it would be best if we only got and installed a-c-r from Git. This would make it more consistent with the rest of our test jobs, which generally take a git_hash
. It would give us the benefit of being able to test any a-c-r branch against the conformance suite; that would be a nice testing win for other large a-c-r branches. And the current Jenkins code is already checking out the a-c-r source from Git; it's just doing less with it. I don't think it would be much work to have it install and run from a virtualenv like arvbox does now.
If it helps, I'd be happy to make the Jenkins changes, it's the same darn virtualenv setup code I've written half a dozen times already.
Thanks.
Updated by Peter Amstutz 24 days ago
Brett Smith wrote in #note-11:
This gets arvados-cwl-runner from our packages and cwltest from pip. For cwltest to access Keep, an appropriate version of arvados-cwl-runner needs to be installed in the same environment as cwltest. We could get arvados-cwl-runner from git, or publish an appropriate dev package to PyPi, but these are not exactly ideal solutions as we end up with two different copies of arvados-cwl-runner which creates potential for confusion.
I think it would be best if we only got and installed a-c-r from Git. This would make it more consistent with the rest of our test jobs, which generally take a
git_hash
. It would give us the benefit of being able to test any a-c-r branch against the conformance suite; that would be a nice testing win for other large a-c-r branches. And the current Jenkins code is already checking out the a-c-r source from Git; it's just doing less with it. I don't think it would be much work to have it install and run from a virtualenv like arvbox does now.If it helps, I'd be happy to make the Jenkins changes, it's the same darn virtualenv setup code I've written half a dozen times already.
Thanks.
The rationale for using packages was to confirm that the OS packages as built work correctly.
The rationale for running the full test suite instead of a single job was that the cloud install uses more complex infrastructure than the containerized test (because all of this already runs as https://ci.arvados.org/view/CWL/job/arvados-cwl-conformance-tests/), so more comprehensive testing is more likely to turn up obscure problems.
I'm happy to hear counter-arguments that one or both of these have a poor return on investment and we should do something different, but that's the reason it doesn't already do what you're suggesting.
Updated by Peter Amstutz 24 days ago
I should mention that what we did before was build and distribute a arvados-cwltest
package specifically to do testing, but we ditched it at some point. Which is where I got the idea of folding cwltest
into the arvados-cwl-runner
package.
Updated by Brett Smith 24 days ago
Okay, so working backwards:
I really don't want to add cwltest to the existing package. I think installing whole new bits of software on all our users' systems just for our testing convenience is a bad habit to be in. And it'll be one of those weird one-off exceptions in the package-building script that becomes inscrutable over time.
I don't think reintroducing the separate cwltest package will meet your needs either, because at least the way we were packaging that at the time, it was in a whole separate venv.
Testing the packaged version is a totally laudable goal (although it feels organizationally weird we're not doing that in the package tests, since that's kind of what they're there for and they can test packages before publication). What if we modified the Jenkins job to work like this:
- If you don't specify a package version, it builds a new venv and installs everything from source (it already takes a
git_hash
so there's not even anything to add) - If you do specify a version, it installs that package version like now, then installs cwltest inside the venv at
/usr/lib/python3-arvados-cwl-runner
. Obviously this is not something you would want to do on a production system, but this isn't a production system so it's fine. And still straightforward.
IMO this meets all the existing needs, gets the Jenkins job working at least a little more like other test jobs, and the scripting is straightforward.
Updated by Peter Amstutz 24 days ago
Brett Smith wrote in #note-14:
Testing the packaged version is a totally laudable goal (although it feels organizationally weird we're not doing that in the package tests, since that's kind of what they're there for and they can test packages before publication). What if we modified the Jenkins job to work like this:
Mainly because doing another 40 minutes of testing would make package building take forever (right now the package tests are just "can the package be installed" and "does the program start without immediately crashing") and the purpose of having "dev" packages is to be able to do further testing.
- If you don't specify a package version, it builds a new venv and installs everything from source (it already takes a
git_hash
so there's not even anything to add)- If you do specify a version, it installs that package version like now, then installs cwltest inside the venv at
/usr/lib/python3-arvados-cwl-runner
. Obviously this is not something you would want to do on a production system, but this isn't a production system so it's fine. And still straightforward.
Installing it into a-c-r's venv is a nice workaround. Here is what it is doing now:
ACR_VERSION=$(./build/get-package-version.sh python3 sdk/cwl) CLIENT_VERSION=$(./build/get-package-version.sh python3 sdk/python) # Need to install the client package separately because the a-c-r package on its own # doesn't link tools like arv-get into /usr/bin sudo apt-get install -y python3-arvados-cwl-runner=$ACR_VERSION python3-arvados-python-client=$CLIENT_VERSION # install cwltest into the same venv used by a-c-r, because cwltest starting with 2.5.20241122133319 # looks for a Keep filesystem plugin that is exported by the a-c-r package as an entry point. sudo /usr/lib/python3-arvados-cwl-runner/bin/pip3 install 'cwltest >=2.5.20241122133319,<3' sudo ln -sf /usr/lib/python3-arvados-cwl-runner/bin/cwltest /usr/bin/cwltest
As you can see, it is deriving the package version from the checked out version (determined by git_hash). Note that for this to work, arvados/jobs needs to have been built, and arvados/jobs is built from packages, so it only makes sense to run this using commits that have already been through the packaging process.
Updated by Brett Smith 19 days ago
Peter Amstutz wrote in #note-15:
Mainly because doing another 40 minutes of testing would make package building take forever (right now the package tests are just "can the package be installed" and "does the program start without immediately crashing")
IMO the very limited scope of our current package testing is a problem, not a virtue. It is a contributing factor to bugs like #22349.
LGTM.
Updated by Peter Amstutz 18 days ago
- Status changed from In Progress to Resolved