From 21a82aa5ae4039d7323f82c594d31ebbbdc5878f Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 8 Nov 2024 16:55:08 +0000 Subject: [PATCH 01/10] Make the container directive optional when using Azure Batch Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../main/nextflow/cloud/azure/batch/AzBatchService.groovy | 7 +++++-- .../nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy | 7 ------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy index cd3e4b37f5..e757db2ccd 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy @@ -393,7 +393,7 @@ class AzBatchService implements Closeable { final container = task.getContainer() if( !container ) - throw new IllegalArgumentException("Missing container image for process: $task.name") + log.warn "Missing container image for process: $task.name" final taskId = "nf-${task.hash.toString()}" // get the pool config final pool = getPoolSpec(poolId) @@ -419,8 +419,11 @@ class AzBatchService implements Closeable { } } // config overall container settings - final containerOpts = new BatchTaskContainerSettings(container) + BatchTaskContainerSettings containerOpts = null + if (container) { + containerOpts = new BatchTaskContainerSettings(container) .setContainerRunOptions(opts) + } // submit command line final String cmd = fusionEnabled ? launcher.fusionSubmitCli(task).join(' ') diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy index 581bed4ec7..50832f2129 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy @@ -60,7 +60,6 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { this.outputFile = task.workDir.resolve(TaskRun.CMD_OUTFILE) this.errorFile = task.workDir.resolve(TaskRun.CMD_ERRFILE) this.exitFile = task.workDir.resolve(TaskRun.CMD_EXIT) - validateConfiguration() } /** only for testing purpose - DO NOT USE */ @@ -70,12 +69,6 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { return executor.batchService } - void validateConfiguration() { - if (!task.container) { - throw new ProcessUnrecoverableException("No container image specified for process $task.name -- Either specify the container to use in the process definition or with 'process.container' value in your config") - } - } - protected BashWrapperBuilder createBashWrapper() { fusionEnabled() ? fusionLauncher() From 8fca02a5fb0b97f9b83bbee84a6d408c5bb3ea5c Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 8 Nov 2024 17:14:30 +0000 Subject: [PATCH 02/10] Remove null container from task spec to make cleaner report in Azure Batch Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../nextflow/cloud/azure/batch/AzBatchService.groovy | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy index e757db2ccd..e8f6415a59 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy @@ -437,15 +437,18 @@ class AzBatchService implements Closeable { log.trace "[AZURE BATCH] Submitting task: $taskId, cpus=${task.config.getCpus()}, mem=${task.config.getMemory()?:'-'}, slots: $slots" - return new BatchTaskCreateContent(taskId, cmd) + final batchTask = new BatchTaskCreateContent(taskId, cmd) .setUserIdentity(userIdentity(pool.opts.privileged, pool.opts.runAs, AutoUserScope.TASK)) - .setContainerSettings(containerOpts) .setResourceFiles(resourceFileUrls(task, sas)) .setOutputFiles(outputFileUrls(task, sas)) .setRequiredSlots(slots) .setConstraints(constraints) - + if (containerOpts) { + batchTask.setContainerSettings(containerOpts) + } + + return batchTask } AzTaskKey runTask(String poolId, String jobId, TaskRun task) { From 3a1cbaf68533c1b31ab53202be48896ac92957f7 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 22 Nov 2024 17:51:02 +0000 Subject: [PATCH 03/10] update azure batch tests Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../azure/batch/AzBatchServiceTest.groovy | 76 +++++++++++++++++++ .../azure/batch/AzBatchTaskHandlerTest.groovy | 8 +- 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy index eceeb644b2..35897374f4 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy @@ -739,4 +739,80 @@ class AzBatchServiceTest extends Specification { [managedIdentity: [clientId: 'client-123']] | 'client-123' } + def 'should create task for submit without container' () { + given: + Global.session = Mock(Session) { getConfig()>>[:] } + and: + def POOL_ID = 'my-pool' + def SAS = '123' + def CONFIG = [storage: [sasToken: SAS]] + def exec = Mock(AzBatchExecutor) {getConfig() >> new AzConfig(CONFIG) } + AzBatchService azure = Spy(new AzBatchService(exec)) + and: + def TASK = Mock(TaskRun) { + getHash() >> HashCode.fromInt(1) + getContainer() >> null + getConfig() >> Mock(TaskConfig) + } + and: + def SPEC = new AzVmPoolSpec(poolId: POOL_ID, vmType: Mock(AzVmType), opts: new AzPoolOpts([:])) + + when: + def result = azure.createTask(POOL_ID, 'salmon', TASK) + then: + 1 * azure.getPoolSpec(POOL_ID) >> SPEC + 1 * azure.computeSlots(TASK, SPEC) >> 4 + 1 * azure.resourceFileUrls(TASK, SAS) >> [] + 1 * azure.outputFileUrls(TASK, SAS) >> [] + and: + result.id == 'nf-01000000' + result.requiredSlots == 4 + and: + result.commandLine == "sh -c 'bash .command.run 2>&1 | tee .command.log'" + and: + result.containerSettings == null + } + + def 'should create task for submit with container and fusion' () { + given: + def SAS = '1234567890' * 10 + def AZURE = [storage: [sasToken: SAS, accountName: 'my-account']] + Global.session = Mock(Session) { getConfig()>>[fusion:[enabled:true], azure: AZURE] } + def WORKDIR = FileSystemPathFactory.parse('az://foo/work/dir') + and: + def POOL_ID = 'my-pool' + def exec = Mock(AzBatchExecutor) {getConfig() >> new AzConfig(AZURE) } + AzBatchService azure = Spy(new AzBatchService(exec)) + and: + def TASK = Mock(TaskRun) { + getHash() >> HashCode.fromInt(1) + getContainer() >> 'ubuntu:latest' + getConfig() >> Mock(TaskConfig) + getWorkDir() >> WORKDIR + toTaskBean() >> Mock(TaskBean) { + getWorkDir() >> WORKDIR + getInputFiles() >> [:] + } + } + and: + def SPEC = new AzVmPoolSpec(poolId: POOL_ID, vmType: Mock(AzVmType), opts: new AzPoolOpts([:])) + + when: + def result = azure.createTask(POOL_ID, 'salmon', TASK) + then: + 1 * azure.getPoolSpec(POOL_ID) >> SPEC + 1 * azure.computeSlots(TASK, SPEC) >> 1 + 1 * azure.resourceFileUrls(TASK, SAS) >> [] + 1 * azure.outputFileUrls(TASK, SAS) >> [] + and: + result.id == 'nf-01000000' + result.requiredSlots == 1 + and: + result.commandLine == "/usr/bin/fusion bash /fusion/az/foo/work/dir/.command.run" + and: + result.containerSettings.imageName == 'ubuntu:latest' + result.containerSettings.containerRunOptions.contains('--privileged') + result.containerSettings.containerRunOptions.contains('-e FUSION_WORK=/fusion/az/foo/work/dir') + } + } diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index 6d0b40d4c4..fbadbc6fdd 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -18,16 +18,14 @@ import spock.lang.Specification */ class AzBatchTaskHandlerTest extends Specification { - def 'should validate config' () { + def 'should validate config with and without container' () { when: - def task = Mock(TaskRun) { getName() >> 'foo'; } + def task = Mock(TaskRun) { getName() >> 'foo' } and: new AzBatchTaskHandler(task: task) .validateConfiguration() then: - def e = thrown(ProcessUnrecoverableException) - e.message.startsWith('No container image specified for process foo') - + noExceptionThrown() when: task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } From 12965bff31a743381e6bd30cec6fecb3409c7214 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 22 Nov 2024 17:55:53 +0000 Subject: [PATCH 04/10] remove test because method is removed Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../azure/batch/AzBatchTaskHandlerTest.groovy | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index fbadbc6fdd..4d2aa5c3e0 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -18,24 +18,6 @@ import spock.lang.Specification */ class AzBatchTaskHandlerTest extends Specification { - def 'should validate config with and without container' () { - when: - def task = Mock(TaskRun) { getName() >> 'foo' } - and: - new AzBatchTaskHandler(task: task) - .validateConfiguration() - then: - noExceptionThrown() - - when: - task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } - and: - new AzBatchTaskHandler(task: task) - .validateConfiguration() - then: - noExceptionThrown() - } - def 'should submit task' () { given: def builder = Mock(BashWrapperBuilder) From ad0755584a4dce9422c5e37d767d366bc5c5eeb2 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:24:35 +0000 Subject: [PATCH 05/10] Can't run gradle on the train so I'm just gonna send it Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../cloud/azure/batch/AzBatchService.groovy | 4 +- .../azure/batch/AzBatchTaskHandler.groovy | 7 ++++ .../cloud/azure/config/AzBatchOpts.groovy | 2 + .../azure/batch/AzBatchTaskHandlerTest.groovy | 39 +++++++++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy index e8f6415a59..66ae83723f 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchService.groovy @@ -392,8 +392,8 @@ class AzBatchService implements Closeable { throw new IllegalArgumentException("Missing Azure Blob storage SAS token") final container = task.getContainer() - if( !container ) - log.warn "Missing container image for process: $task.name" + if( !container && config.batch().requireContainer ) + throw new IllegalArgumentException("Missing container image for process: $task.name\nYou can disable this behaviour setting `azure.batch.requireContainer=false` in the nextflow config file") final taskId = "nf-${task.hash.toString()}" // get the pool config final pool = getPoolSpec(poolId) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy index 50832f2129..594577b43c 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy @@ -60,6 +60,7 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { this.outputFile = task.workDir.resolve(TaskRun.CMD_OUTFILE) this.errorFile = task.workDir.resolve(TaskRun.CMD_ERRFILE) this.exitFile = task.workDir.resolve(TaskRun.CMD_EXIT) + validateConfiguration() } /** only for testing purpose - DO NOT USE */ @@ -69,6 +70,12 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { return executor.batchService } + void validateConfiguration() { + if (!task.container && config.batch().requireContainer ) { + throw new ProcessUnrecoverableException("No container image specified for process $task.name -- Either specify the container to use in the process definition or with 'process.container' value in your config. You can disable this behaviour setting `azure.batch.requireContainer=false` in the nextflow config file") + } + } + protected BashWrapperBuilder createBashWrapper() { fusionEnabled() ? fusionLauncher() diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy index 358c70413d..efb68b51c0 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy @@ -54,6 +54,7 @@ class AzBatchOpts implements CloudTransferOptions { Boolean deleteJobsOnCompletion Boolean deletePoolsOnCompletion Boolean deleteTasksOnCompletion + Boolean requireContainer CopyToolInstallMode copyToolInstallMode Map pools @@ -67,6 +68,7 @@ class AzBatchOpts implements CloudTransferOptions { location = config.location autoPoolMode = config.autoPoolMode allowPoolCreation = config.allowPoolCreation + requireContainer = config.requireContainer ?: true terminateJobsOnCompletion = config.terminateJobsOnCompletion != Boolean.FALSE deleteJobsOnCompletion = config.deleteJobsOnCompletion deletePoolsOnCompletion = config.deletePoolsOnCompletion diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index 4d2aa5c3e0..70864d7c95 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -18,6 +18,45 @@ import spock.lang.Specification */ class AzBatchTaskHandlerTest extends Specification { + def 'should validate config' () { + when: + def task = Mock(TaskRun) { getName() >> 'foo'; } + and: + new AzBatchTaskHandler(task: task) + .validateConfiguration() + then: + def e = thrown(ProcessUnrecoverableException) + e.message.startsWith('No container image specified for process foo') + + + when: + task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } + and: + new AzBatchTaskHandler(task: task) + .validateConfiguration() + then: + noExceptionThrown() + } + + def 'should ignore missing container if disabled' () { + when: + def task = Mock(TaskRun) { getName() >> 'foo'; } + and: + new AzBatchTaskHandler(task: task, config: new AzConfig([batch: [requireContainer: false]])) + .validateConfiguration() + then: + noExceptionThrown() + + + when: + task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } + and: + new AzBatchTaskHandler(task: task) + .validateConfiguration() + then: + noExceptionThrown() + } + def 'should submit task' () { given: def builder = Mock(BashWrapperBuilder) From 536568c30d96af8c3f802afaf20b19d38614559b Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 6 Dec 2024 12:51:59 +0000 Subject: [PATCH 06/10] Need to fixup testss Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy | 2 +- .../cloud/azure/batch/AzBatchTaskHandlerTest.groovy | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy index 594577b43c..2da2baa6bf 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy @@ -71,7 +71,7 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { } void validateConfiguration() { - if (!task.container && config.batch().requireContainer ) { + if (!task.container && executor.config.batch().requireContainer ) { throw new ProcessUnrecoverableException("No container image specified for process $task.name -- Either specify the container to use in the process definition or with 'process.container' value in your config. You can disable this behaviour setting `azure.batch.requireContainer=false` in the nextflow config file") } } diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index 70864d7c95..b4988ff913 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -1,5 +1,6 @@ package nextflow.cloud.azure.batch +import nextflow.cloud.azure.config.AzConfig import nextflow.cloud.types.CloudMachineInfo import nextflow.cloud.types.PriceModel import nextflow.exception.ProcessUnrecoverableException @@ -21,6 +22,9 @@ class AzBatchTaskHandlerTest extends Specification { def 'should validate config' () { when: def task = Mock(TaskRun) { getName() >> 'foo'; } + def CONFIG = [batch: [requireContainer: false]] + def exec = Mock(Executor) { getName() >> 'azurebatch' } + processor.getExecutor() >> exec and: new AzBatchTaskHandler(task: task) .validateConfiguration() @@ -41,8 +45,9 @@ class AzBatchTaskHandlerTest extends Specification { def 'should ignore missing container if disabled' () { when: def task = Mock(TaskRun) { getName() >> 'foo'; } + def CONFIG = [batch: [requireContainer: false]] and: - new AzBatchTaskHandler(task: task, config: new AzConfig([batch: [requireContainer: false]])) + new AzBatchTaskHandler(task: task, config: new AzConfig(CONFIG)) .validateConfiguration() then: noExceptionThrown() From 762100b29ad98bfc5f10e47b6f782241d339299d Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 6 Dec 2024 13:35:12 +0000 Subject: [PATCH 07/10] fixup tests some more Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../azure/batch/AzBatchServiceTest.groovy | 2 +- .../azure/batch/AzBatchTaskHandlerTest.groovy | 56 +++++++++++++------ 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy index 35897374f4..45b58011bb 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchServiceTest.groovy @@ -745,7 +745,7 @@ class AzBatchServiceTest extends Specification { and: def POOL_ID = 'my-pool' def SAS = '123' - def CONFIG = [storage: [sasToken: SAS]] + def CONFIG = [storage: [sasToken: SAS], batch: [requireContainer: false]] def exec = Mock(AzBatchExecutor) {getConfig() >> new AzConfig(CONFIG) } AzBatchService azure = Spy(new AzBatchService(exec)) and: diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index b4988ff913..c57f7126e7 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -21,43 +21,63 @@ class AzBatchTaskHandlerTest extends Specification { def 'should validate config' () { when: - def task = Mock(TaskRun) { getName() >> 'foo'; } - def CONFIG = [batch: [requireContainer: false]] - def exec = Mock(Executor) { getName() >> 'azurebatch' } - processor.getExecutor() >> exec + def task = Mock(TaskRun) { + getName() >> 'foo' + getProcessor() >> Mock(TaskProcessor) { + getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } + } + } + def CONFIG = [batch: [requireContainer: true]] and: - new AzBatchTaskHandler(task: task) - .validateConfiguration() + new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { + getConfig() >> new AzConfig(CONFIG) + }).validateConfiguration() then: def e = thrown(ProcessUnrecoverableException) e.message.startsWith('No container image specified for process foo') - when: - task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } + task = Mock(TaskRun) { + getName() >> 'foo' + getContainer() >> 'ubuntu' + } and: - new AzBatchTaskHandler(task: task) - .validateConfiguration() + new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { + getConfig() >> new AzConfig(CONFIG) + }).validateConfiguration() then: noExceptionThrown() } def 'should ignore missing container if disabled' () { when: - def task = Mock(TaskRun) { getName() >> 'foo'; } - def CONFIG = [batch: [requireContainer: false]] + def task = Mock(TaskRun) { + getName() >> 'foo' + getContainer() >> null + getProcessor() >> Mock(TaskProcessor) { + getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } + } + } + def CONFIG = [requireContainer: false] and: - new AzBatchTaskHandler(task: task, config: new AzConfig(CONFIG)) - .validateConfiguration() + new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { + getConfig() >> new AzConfig(CONFIG) + }).validateConfiguration() then: noExceptionThrown() - when: - task = Mock(TaskRun) { getName() >> 'foo'; getContainer() >> 'ubuntu' } + task = Mock(TaskRun) { + getName() >> 'foo' + getContainer() >> 'ubuntu' + getProcessor() >> Mock(TaskProcessor) { + getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } + } + } and: - new AzBatchTaskHandler(task: task) - .validateConfiguration() + new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { + getConfig() >> new AzConfig([:]) + }).validateConfiguration() then: noExceptionThrown() } From dd336aba62bffb3be33419dbe54186bf91c32277 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Fri, 6 Dec 2024 16:14:33 +0000 Subject: [PATCH 08/10] fixup tests some more Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../azure/batch/AzBatchTaskHandler.groovy | 1 + .../azure/batch/AzBatchTaskHandlerTest.groovy | 43 +++++++------------ 2 files changed, 17 insertions(+), 27 deletions(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy index 2da2baa6bf..06448b9fd2 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/batch/AzBatchTaskHandler.groovy @@ -71,6 +71,7 @@ class AzBatchTaskHandler extends TaskHandler implements FusionAwareTask { } void validateConfiguration() { + println "AZURE_BATCH_CONFIG: ${executor.config.batch()}" if (!task.container && executor.config.batch().requireContainer ) { throw new ProcessUnrecoverableException("No container image specified for process $task.name -- Either specify the container to use in the process definition or with 'process.container' value in your config. You can disable this behaviour setting `azure.batch.requireContainer=false` in the nextflow config file") } diff --git a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy index c57f7126e7..fbecf1b8be 100644 --- a/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy +++ b/plugins/nf-azure/src/test/nextflow/cloud/azure/batch/AzBatchTaskHandlerTest.groovy @@ -27,10 +27,9 @@ class AzBatchTaskHandlerTest extends Specification { getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } } } - def CONFIG = [batch: [requireContainer: true]] and: new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { - getConfig() >> new AzConfig(CONFIG) + getConfig() >> new AzConfig([:]) }).validateConfiguration() then: def e = thrown(ProcessUnrecoverableException) @@ -41,43 +40,33 @@ class AzBatchTaskHandlerTest extends Specification { getName() >> 'foo' getContainer() >> 'ubuntu' } + def CONFIG = [batch: [requireContainer: true]] and: new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { - getConfig() >> new AzConfig(CONFIG) + getConfig() >> new AzConfig(CONFIG) }).validateConfiguration() then: noExceptionThrown() } def 'should ignore missing container if disabled' () { - when: - def task = Mock(TaskRun) { - getName() >> 'foo' - getContainer() >> null - getProcessor() >> Mock(TaskProcessor) { - getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } - } + given: + def task = Mock(TaskRun) + task.getName() >> 'batch-task' + task.getConfig() >> new TaskConfig() + task.getProcessor() >> Mock(TaskProcessor) { + getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } } - def CONFIG = [requireContainer: false] and: - new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { - getConfig() >> new AzConfig(CONFIG) - }).validateConfiguration() - then: - noExceptionThrown() + def handler = Spy(AzBatchTaskHandler) + handler.task = task + handler.executor = Mock(AzBatchExecutor) { + getConfig() >> new AzConfig([batch: [requireContainer: false]]) + } when: - task = Mock(TaskRun) { - getName() >> 'foo' - getContainer() >> 'ubuntu' - getProcessor() >> Mock(TaskProcessor) { - getExecutor() >> Mock(Executor) { getName() >> 'azurebatch' } - } - } - and: - new AzBatchTaskHandler(task: task, executor: Mock(AzBatchExecutor) { - getConfig() >> new AzConfig([:]) - }).validateConfiguration() + handler.validateConfiguration() + then: noExceptionThrown() } From 391dc5cfaa2eba82b7fb5a0f0c28f689db48eb29 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Mon, 9 Dec 2024 17:54:45 +0000 Subject: [PATCH 09/10] parse the config option correctly Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- .../src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy index efb68b51c0..cd11f88161 100644 --- a/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy +++ b/plugins/nf-azure/src/main/nextflow/cloud/azure/config/AzBatchOpts.groovy @@ -68,7 +68,7 @@ class AzBatchOpts implements CloudTransferOptions { location = config.location autoPoolMode = config.autoPoolMode allowPoolCreation = config.allowPoolCreation - requireContainer = config.requireContainer ?: true + requireContainer = config.requireContainer == null ? true : config.requireContainer terminateJobsOnCompletion = config.terminateJobsOnCompletion != Boolean.FALSE deleteJobsOnCompletion = config.deleteJobsOnCompletion deletePoolsOnCompletion = config.deletePoolsOnCompletion From d88452b535b18392be937fa69c0767f00adba3b3 Mon Sep 17 00:00:00 2001 From: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> Date: Mon, 9 Dec 2024 17:58:07 +0000 Subject: [PATCH 10/10] Document the azure.batch.requireContainer config option Signed-off-by: adamrtalbot <12817534+adamrtalbot@users.noreply.github.com> --- docs/reference/config.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/reference/config.md b/docs/reference/config.md index 0efb1ce6ae..80b39c355b 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -395,6 +395,11 @@ The following settings are available: `azure.batch.pools..vmType` : Specify the virtual machine type used by the pool identified with ``. +`azure.batch.requireContainer` +: :::{versionadded} 25.01.0-edge + ::: +: Require the use of a container when running tasks (default: `true`). Note this requires an existing Azure Batch pool configured to not use a container image. Running a process without a container when the node pool requires one will raise an error on Azure Batch + `azure.managedIdentity.clientId` : Specify the client ID for an Azure [managed identity](https://learn.microsoft.com/en-us/entra/identity/managed-identities-azure-resources/overview). See {ref}`azure-managed-identities` for more details.