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

fixed conda lockfile rendering in build view #727

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

munishchouhan
Copy link
Member

This PR will add a check on conda lockfile and if its not correct, wave will fetch it from previous succcessful build

Signed-off-by: munishchouhan <[email protected]>
@munishchouhan munishchouhan linked an issue Oct 29, 2024 that may be closed by this pull request
@munishchouhan munishchouhan self-assigned this Oct 29, 2024
@munishchouhan
Copy link
Member Author

tested:
Screenshot 2024-10-29 at 13 08 28
Screenshot 2024-10-29 at 13 08 17

@munishchouhan
Copy link
Member Author

download tested
Screenshot 2024-10-29 at 13 22 09

Signed-off-by: munishchouhan <[email protected]>
@munishchouhan munishchouhan changed the title fixed conda file rendering fixed conda lockfile rendering in build view Oct 29, 2024
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
Copy link
Collaborator

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

I'm not convinced but this solution because still keep the problem unsolved for the stream API.

Also, this not prevent the upload of lock files that just contains the invalid content.

Instead of patching the view of the broken conda file containing cat environmeny.lock string, it would have more sense to move this logic when *uploading* the conda lock content.

@munishchouhan
Copy link
Member Author

Instead of patching the view of the broken conda file containing cat environmeny.lock string, it would have more sense to move this logic when uploading the conda lock content.

Sure, I will move the logic in uploading section

@munishchouhan
Copy link
Member Author

Tested again after changes:
Screenshot 2024-12-10 at 18 36 22
Screenshot 2024-12-10 at 18 36 37
Screenshot 2024-12-10 at 18 35 21

Signed-off-by: munishchouhan <[email protected]>
Comment on lines 157 to 175

/* When a container image is cached, dockerfile does not get executed.
In that case condalock file will contain "cat environment.lock" because its not been executed.
So wave will check the previous builds of that container image
and render the condalock file from latest successful build
and replace with the current build's condalock file.
*/
if( condaLock.contains('cat environment.lock') ){
log.info "Container Image is already cached, uploading previously successful build's condalock file for buildId: $buildId"
def builds = persistenceService.allBuilds(buildId.split('-')[1].split('_')[0])
for (def build : builds) {
if ( build.succeeded() ){
def curCondaLock = fetchCondaLockString(build.buildId)
if( curCondaLock && !curCondaLock.contains('cat environment.lock') ){
condaLock = curCondaLock
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's isolate this logic into a separate method, so it can be tested independently

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: munishchouhan <[email protected]>
@munishchouhan
Copy link
Member Author

Tests again locally:
Logs:

10:26:31.870 [jobs-queue-thread-0] INFO  i.s.w.s.b.i.ContainerBuildServiceImpl - == Container build completed 'hrma017/dev:bwa--ed273bf3f9df190c' - operation=bd-ed273bf3f9df190c-2; exit=0; status=SUCCEEDED; duration=PT22.782941S
10:26:31.874 [io-executor-thread-4] DEBUG i.s.w.s.logs.BuildLogServiceImpl - Storing logs for buildId: bd-ed273bf3f9df190c_2
10:26:32.464 [io-executor-thread-4] INFO  i.s.w.s.logs.BuildLogServiceImpl - Container Image is already cached, uploading previously successful build's condalock file for buildId: bd-ed273bf3f9df190c_2
10:26:32.861 [io-executor-thread-4] DEBUG i.s.w.s.logs.BuildLogServiceImpl - Storing conda lock for buildId: bd-ed273bf3f9df190c_2

First build:
Screenshot 2024-12-11 at 10 23 50

Second Build:
Screenshot 2024-12-11 at 10 28 24

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.

Conda lock file may not be rendered correctly
2 participants