-
Notifications
You must be signed in to change notification settings - Fork 703
feat: Azure Batch eagerly terminates jobs after all tasks have been submitted #6159
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
Open
adamrtalbot
wants to merge
6
commits into
master
Choose a base branch
from
5839_azure_batch_jobs_terminate_upon_completion
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
92c58a6
feat: Azure Batch eagerly terminates jobs after all tasks have been s…
adamrtalbot 3cb3598
Correct copywright notice
adamrtalbot 07c8a70
Cody simplification and tidy
adamrtalbot cb2b7df
Merge branch 'master' into 5839_azure_batch_jobs_terminate_upon_compl…
adamrtalbot 9d93c91
Merge branch 'master' into 5839_azure_batch_jobs_terminate_upon_compl…
pditommaso 46c0b63
Merge branch 'master' into 5839_azure_batch_jobs_terminate_upon_compl…
adamrtalbot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
74 changes: 74 additions & 0 deletions
74
plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchProcessObserver.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
/* | ||
* Copyright 2025, Seqera | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package nextflow.cloud.azure.batch | ||
|
||
import groovy.transform.CompileStatic | ||
import groovy.util.logging.Slf4j | ||
import nextflow.Session | ||
import nextflow.processor.TaskProcessor | ||
import nextflow.trace.TraceObserver | ||
|
||
/** | ||
* Observer that handles process termination events for Azure Batch executor. | ||
* When a process terminates (all tasks have been submitted), this observer | ||
* will eagerly set the corresponding Azure Batch job to auto-terminate when | ||
* all tasks complete. | ||
* | ||
* @author Adam Talbot <[email protected]> | ||
*/ | ||
@Slf4j | ||
@CompileStatic | ||
class AzBatchProcessObserver implements TraceObserver { | ||
|
||
AzBatchProcessObserver(Session session) { | ||
// Session not needed, but required by factory interface | ||
} | ||
|
||
/** | ||
* Called when a process terminates (all tasks have been submitted). | ||
* Sets Azure Batch jobs to auto-terminate when all tasks complete. | ||
*/ | ||
@Override | ||
void onProcessTerminate(TaskProcessor processor) { | ||
// Check if this process uses the Azure Batch executor | ||
if( !(processor.executor instanceof AzBatchExecutor) ) { | ||
return | ||
} | ||
|
||
final executor = processor.executor as AzBatchExecutor | ||
final batchService = executor.batchService | ||
|
||
// Check if auto-termination is enabled | ||
if( !batchService?.config?.batch()?.terminateJobsOnCompletion ) { | ||
log.trace "Azure Batch job auto-termination is disabled, skipping eager termination for process: ${processor.name}" | ||
return | ||
} | ||
|
||
// Find and set auto-termination for all jobs associated with this processor | ||
batchService.allJobIds.findAll { key, jobId -> | ||
key.processor == processor | ||
}.values().each { jobId -> | ||
Comment on lines
+52
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels a bit convoluted to me...is there an easier way? |
||
log.debug "Setting Azure Batch job ${jobId} to auto-terminate for completed process: ${processor.name}" | ||
try { | ||
batchService.setJobAutoTermination(jobId) | ||
} | ||
catch( Exception e ) { | ||
log.warn "Failed to set auto-termination for Azure Batch job ${jobId} associated with process '${processor.name}' - ${e.message ?: e}" | ||
} | ||
} | ||
} | ||
} |
37 changes: 37 additions & 0 deletions
37
plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchProcessObserverFactory.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright 2025, Seqera | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package nextflow.cloud.azure.batch | ||
|
||
import groovy.transform.CompileStatic | ||
import nextflow.Session | ||
import nextflow.trace.TraceObserver | ||
import nextflow.trace.TraceObserverFactory | ||
|
||
/** | ||
* Factory for creating the Azure Batch process observer that enables eager termination | ||
* of Azure Batch jobs when processes complete. | ||
* | ||
* @author Adam Talbot <[email protected]> | ||
*/ | ||
@CompileStatic | ||
class AzBatchProcessObserverFactory implements TraceObserverFactory { | ||
|
||
@Override | ||
Collection<TraceObserver> create(Session session) { | ||
return [new AzBatchProcessObserver(session)] | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
89 changes: 89 additions & 0 deletions
89
plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchProcessObserverTest.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright 2025, Seqera | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package nextflow.cloud.azure.batch | ||
|
||
import nextflow.Session | ||
import nextflow.cloud.azure.config.AzBatchOpts | ||
import nextflow.cloud.azure.config.AzConfig | ||
import nextflow.executor.Executor | ||
import nextflow.processor.TaskProcessor | ||
import spock.lang.Specification | ||
|
||
/** | ||
* Test for AzBatchProcessObserver | ||
* | ||
* @author Adam Talbot <[email protected]> | ||
*/ | ||
class AzBatchProcessObserverTest extends Specification { | ||
|
||
def 'should only act on Azure Batch executors'() { | ||
given: | ||
def observer = new AzBatchProcessObserver(Mock(Session)) | ||
def processor = Mock(TaskProcessor) { | ||
getExecutor() >> Mock(Executor) // Not an AzBatchExecutor | ||
} | ||
|
||
when: | ||
observer.onProcessTerminate(processor) | ||
|
||
then: | ||
noExceptionThrown() | ||
} | ||
|
||
def 'should set job auto-termination when enabled'() { | ||
given: | ||
def observer = new AzBatchProcessObserver(Mock(Session)) | ||
def processor = Mock(TaskProcessor) { getName() >> 'test-process' } | ||
def batchService = Mock(AzBatchService) { | ||
getConfig() >> Mock(AzConfig) { | ||
batch() >> Mock(AzBatchOpts) { | ||
terminateJobsOnCompletion >> true | ||
} | ||
} | ||
getAllJobIds() >> [(new AzJobKey(processor, 'pool1')): 'job123'] | ||
} | ||
def executor = Mock(AzBatchExecutor) { getBatchService() >> batchService } | ||
processor.getExecutor() >> executor | ||
|
||
when: | ||
observer.onProcessTerminate(processor) | ||
|
||
then: | ||
1 * batchService.setJobAutoTermination('job123') | ||
} | ||
|
||
def 'should skip when termination disabled'() { | ||
given: | ||
def observer = new AzBatchProcessObserver(Mock(Session)) | ||
def processor = Mock(TaskProcessor) { getName() >> 'test-process' } | ||
def batchService = Mock(AzBatchService) { | ||
getConfig() >> Mock(AzConfig) { | ||
batch() >> Mock(AzBatchOpts) { | ||
terminateJobsOnCompletion >> false | ||
} | ||
} | ||
} | ||
def executor = Mock(AzBatchExecutor) { getBatchService() >> batchService } | ||
processor.getExecutor() >> executor | ||
|
||
when: | ||
observer.onProcessTerminate(processor) | ||
|
||
then: | ||
0 * batchService.setJobAutoTermination(_) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why using an observer instead of having this logic in the task hander?
nextflow/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy
Line 127 in 08c0027
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.
It's not a task, it's a job!
In Azure, job = queue.
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.
See comment here: #3927 (comment)
Basically, it needs to wait until the last task of a process has been submitted to Az Batch, then make the job terminate after completion.
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.
Fair, but I still don't see a big value using the trace observer. Would not make more sense to keep this in the cleanup logic here
https://github.com/nextflow-io/nextflow/blob/5839_azure_batch_jobs_terminate_upon_completion/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy#L1065-L1080
all related metadata should be accessible in the AzBatchService object
Uh oh!
There was an error while loading. Please reload this page.
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.
That is how it currently works, see terminateJobsOnCompletion and deleteJobsOnCompletion. However it's causing us problems. If Nextflow dies, all jobs are left in active state which consumes a very limited quota. For some context, I had to argue with Azure support a lot to get 1000 active jobs as a quota, and with 1 process equaling 1 job you can use this up in a matter of days. Once you have done this, the only way to run Nextflow again is to go into your Azure Batch account and manually remove some jobs.
By aggressively going and setting jobs to automatically terminate, we can reduce the number of active jobs to all but the ones with running tasks when a Nextflow process dies. This is reducing the pressure on active jobs as much as possible. Between this and the 30 day cooldown, we should be at effectively zero active jobs for normal running which is what we should aim for.
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.
Then not sure how much this approach will improve, most of times processes termination on pipeline completion, so if the execution is killed abruptly the behaviour will be more or less the same.
I wonder instead if it could be a problem with the cleanup execution. Are you able to replicate the problem? do you have any execution logs for effected pipelines ?
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.
Arrgghh that's frustrating.
Attached one from last night.
nf-5ZBk4BU4tGxbqU.log
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.
It sounds like the earliest point at which we could safely terminate the azure job is right after all tasks for the corresponding process have been submitted to azure batch.
The TaskProcessor can signal when all tasks are "pending" (submitted to nextflow but not to azure) and when all tasks have completed, but not when all tasks are submitted to azure. I think the trace observer is the only way to do this, because it needs to watch the task submissions to figure out when all tasks have been submitted.
In fact, as I write this, even that isn't enough because you could have task retries. If I submit all the tasks, terminate the job, then a task fails and I need to retry it, I assume that wouldn't work if the job was already terminated? Therefore we actually do have to wait until all tasks are completed.
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.
Damn, you're right. Back to the drawing board.