-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for conda lock file #642
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
Conversation
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Good start. To tell the truth, still not sure we should go ahead with this approach or just store the lock file in the surreal db like we are doing for the conda env, even tho it there's the possibility to have the same problem as #559 |
I'm bit confused by this comment in the issue
It seems suggesting the default sql body size is 1 MB, instead the error we are hitting is much smaller. |
we are using /key routes, which is constrained by 16 KiB |
if we use sql to store it then we can bypass this limit |
This sounds like a plan. please give it a try |
ok sure |
There is an issue accessing the Conda lock file. The lock file is present in the generated image, not in the buildkit container we are running in Wave. |
in latter case of generating conda lockfile from conda file, we still need another job to achieve that |
Better to get the file from the container - I was trying to avoid generating the lock file separately because then there's no absolute guarantee that it'll end up the same as the actual environment. If it comes from the environment itself it's certain. |
Can we print the lock file to stdout and then capture that from the build? |
we can do this:
I ran it for conda package 'bwa'
|
Signed-off-by: munishchouhan <[email protected]>
Exactly - that works! Were there a load of lines after the We'd need to remove the line prefixes but that's all I think.. |
A better approach (maybe) could be: 1) creating the container "locally"; 2) copy the lock file from the built container via buildkit; 3) uploading it to the registry. Something similar is done for singularity, here. |
Ok sure, I will try this one |
final query = """\ | ||
INSERT into wave_conda_lock { | ||
buildId: '$buildId', | ||
condaLock = '$condaLock' |
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.
what if the lock file contains a '
?
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.
good point
I have not tested yet with surrealDB
I will use bytes datatype to save it
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.
changed to byte[]
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Added |
may be |
Signed-off-by: munishchouhan <[email protected]>
That was missing indeed, now I see the upload but it's not rendering in the view. checking |
ok works |
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@pditommaso is this integration test is enough or were you referring to some other test?
|
Ideally it would nice to have a test passing trough the controller, building a conda container and check the lock file exists. S3 uploading not so important, could be mocked. More important lock file is grabbed. |
I gave it try and able to succeed with full flow, but when we do build in test and image is uploaded to the registry, next time when the same test runs, it will find that image and return cached and in that cache we will not have conda lockfile I will try to mock the build and send mockedevent to do this testing |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
I overcome the caching issue by providing the invalid repo. so the push image part fails and everytime we run the test, it creates a fresh build |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
this e2e test tasks long time to complete, what we can do is add another module for e2e test, which runs on demand and when merging a pr to master |
Ok, let's keep the e2e in a separate PR and merge this one |
yes, working on that, |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
this still requires the update in |
no, already merged 👍 |
Signed-off-by: munishchouhan <[email protected]>
depends upon seqeralabs/libseqera#25
the above PR needs 'git revert d088604' before merging
This PR will add the following