Update nf-azure: Add Azure Batch node communication mode option#7337
Update nf-azure: Add Azure Batch node communication mode option#7337LennyBEL wants to merge 2 commits into
Conversation
Signed-off-by: Lenny Van de Winkel <vdwlenny@outlook.be>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
adamrtalbot
left a comment
adamrtalbot
left a comment
There was a problem hiding this comment.
Nice feature, I agree it's worth having.
Suggestion: expose default as an explicit value alongside simplified/classic. The SDK defines BatchNodeCommunicationMode.DEFAULT, so the enum set would be complete and users get an explicit way to request the Batch service default rather than only reaching it by omitting the option.
Note: default is likely functionally equivalent to leaving the option unset but I'm not certain.
I want one of the core Nextflow devs to check it for style.
| protected static BatchNodeCommunicationMode parseCommunicationMode(value) { | ||
| if( value == null ) | ||
| return null | ||
| if( value instanceof BatchNodeCommunicationMode ) | ||
| return value | ||
| final str = value.toString().toLowerCase() | ||
| if( str == 'simplified' ) return BatchNodeCommunicationMode.SIMPLIFIED | ||
| if( str == 'classic' ) return BatchNodeCommunicationMode.CLASSIC | ||
| throw new IllegalArgumentException("Invalid azure.batch.pools.<name>.targetCommunicationMode value: '$value' - expected 'simplified' or 'classic'") |
There was a problem hiding this comment.
We could include an explicit "default" here:
I've also removed the using BatchNodeCommunication mode because I think it makes the method confusing.
| protected static BatchNodeCommunicationMode parseCommunicationMode(value) { | |
| if( value == null ) | |
| return null | |
| if( value instanceof BatchNodeCommunicationMode ) | |
| return value | |
| final str = value.toString().toLowerCase() | |
| if( str == 'simplified' ) return BatchNodeCommunicationMode.SIMPLIFIED | |
| if( str == 'classic' ) return BatchNodeCommunicationMode.CLASSIC | |
| throw new IllegalArgumentException("Invalid azure.batch.pools.<name>.targetCommunicationMode value: '$value' - expected 'simplified' or 'classic'") | |
| protected static BatchNodeCommunicationMode parseCommunicationMode(value) { | |
| final str = value.toString().toLowerCase() | |
| if( str == 'simplified' ) return BatchNodeCommunicationMode.SIMPLIFIED | |
| if( str == 'classic' ) return BatchNodeCommunicationMode.CLASSIC | |
| if( str == 'default' ) return BatchNodeCommunicationMode.DEFAULT | |
| throw new IllegalArgumentException("Invalid azure.batch.pools.<name>.targetCommunicationMode value: '$value' - expected 'simplified', 'classic' or 'default'") | |
| } |
There was a problem hiding this comment.
Note:
defaultis likely functionally equivalent to leaving the option unset but I'm not certain.
I'm about 99% certain that that's the case. However, I can't find any clear documentation on that...
However, given that this option was never explicitly set up until now, we can safely conclude that leaving it empty means it takes the 'Default' option.
| this.password = opts.password | ||
| this.virtualNetwork = opts.virtualNetwork | ||
| this.lowPriority = opts.lowPriority as boolean | ||
| this.targetCommunicationMode = parseCommunicationMode(opts.targetCommunicationMode) |
There was a problem hiding this comment.
The Nextflow team have the final say on style, but it may be more disciplined to only use parseCommunicationMode when targetCommunicationMode is populated:
| this.targetCommunicationMode = parseCommunicationMode(opts.targetCommunicationMode) | |
| this.targetCommunicationMode = opts.targetCommunicationMode != null ?parseCommunicationMode(opts.targetCommunicationMode) : null |
Style choice, it's been a while since I've contributed so I want one of the core developers to look at it!
christopher-hakkaart
left a comment
christopher-hakkaart
left a comment
There was a problem hiding this comment.
Editorial suggestions to make the text more task orientated.
|
|
||
| ##### `azure.batch.pools.<name>.targetCommunicationMode` | ||
|
|
||
| The node communication mode used by the pool. Can be either `classic` or `simplified`. When not set, the Azure Batch service selects the mode. In `simplified` mode the Batch service initiates communication with the compute nodes, reducing the outbound network access required by the nodes. |
There was a problem hiding this comment.
| The node communication mode used by the pool. Can be either `classic` or `simplified`. When not set, the Azure Batch service selects the mode. In `simplified` mode the Batch service initiates communication with the compute nodes, reducing the outbound network access required by the nodes. | |
| The node communication mode used by the pool. One of `classic` or `simplified`. When not set, the Azure Batch service selects the mode. In `simplified` mode, the Batch service initiates communication with the compute nodes, which reduces the outbound network access the nodes require. | |
| Pools can be configured to use [simplified compute node communication](https://learn.microsoft.com/en-us/azure/batch/simplified-compute-node-communication), which changes how the Batch service communicates with the compute nodes and reduces the outbound network access required by the nodes. | ||
|
|
||
| ```groovy | ||
| azure.batch.pools.<pool-name>.targetCommunicationMode = 'simplified' // 'simplified' or 'classic' | ||
| ``` | ||
|
|
||
| When this option is not set, the Azure Batch service selects the communication mode. |
There was a problem hiding this comment.
| Pools can be configured to use [simplified compute node communication](https://learn.microsoft.com/en-us/azure/batch/simplified-compute-node-communication), which changes how the Batch service communicates with the compute nodes and reduces the outbound network access required by the nodes. | |
| ```groovy | |
| azure.batch.pools.<pool-name>.targetCommunicationMode = 'simplified' // 'simplified' or 'classic' | |
| ``` | |
| When this option is not set, the Azure Batch service selects the communication mode. | |
| To use [simplified compute node communication](https://learn.microsoft.com/en-us/azure/batch/simplified-compute-node-communication), set `targetCommunicationMode` to `simplified`. This changes how the Batch service communicates with the compute nodes and reduces the outbound network access the nodes require. | |
| ```groovy | |
| azure.batch.pools.<pool-name>.targetCommunicationMode = 'simplified' // 'simplified' or 'classic' | |
| ``` | |
| When this option is not set, the Azure Batch service selects the communication mode. |
3 participants
Split from #7321
Feature: Node Communication Mode
In our environment, Azure Batch is configured within a virtual network. However, Nextflow does not expose a configuration item for the Node Communication Mode. The Classic mode has been deprecated for a while, but Nextflow pools still default to it because they use an older API version. Without a configuration item, we cannot explictly tell it to use "simplified" mode.
This PR adds azure.batch.pools..targetCommunicationMode (named to match the Azure Batch API and SDKs). Accepted values are simplified and classic; defaulting to null to let the Azure Batch API choose.
Reference: https://learn.microsoft.com/en-us/azure/batch/simplified-compute-node-communication
Expected Impact
None, as the default behavior does not change at all.
Tests
Unit tests added/updated in
nf-azure:AzPoolOptsTesttargetCommunicationMode, with errors on invalid values.funnel) sensitivity: the pool hash changes when the communication mode changes.AzBatchServiceTestDocs updated:
docs/azure.mdxanddocs/reference/config.mdx.