Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add support in AWS Batch Operator for multinode jobs - #29522

Merged
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch
Apr 12, 2023
Merged

Add support in AWS Batch Operator for multinode jobs#29522
dimberman merged 27 commits into
apache:mainfrom
aws-mwaa:vandonr/batch

Conversation

@vandonr-amz

Copy link
Copy Markdown
Contributor

picking up #28321 after it's been somewhat abandoned by the original author.
Addressed my own comment about empty array, and it should be good to go I think.

Initial description from @camilleanne:

  • Adds support for AWS Batch multinode jobs by allowing a node_overrides json object to be passed through to the boto3 submit_job method.

    • Adds support for multinode jobs by properly parsing the output of describe_jobs (which is different for container vs multinode) to extract the log stream name.

closes: #25522

@Taragolis

Copy link
Copy Markdown
Contributor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job?
node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@vandonr-amz just and idea not a strong opinion. What if we create separate Operator for multinode Job? node_overrides should conflicts with container_overrides (current overrides) and operator have a different logic for obtain logs, in additional BatchOperator use their own implementation of waiter (separate additional hook 🙄 ), I'm not sure is it would work with this changes or not.

I'm not super fan of it, it's the same boto operation behind it, just with a different behavior... And as a user I think I would be surprised if I had to use a different operator for this.
I think an operator should be "do a thing" and then the parameters should be about the specifics of how the thing should be done. I don't think lanching a batch job or a multinode batch job are really different "things", but it's up for debate I guess.

@Taragolis

Taragolis commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

There is 3 different sets of parameters for SubmitJob

  1. containerOverrides which run batch job on either EC2 or Fargate
  2. nodeOverrides which run batch job in EC2. This property include own containerOverrides
  3. eksPropertiesOverride run batch job on EKS cluster

I also guess that arrayProperties only applicable for containerOverrides

IMHO SubmitJob it is pretty complicated. One potential benefit for keep all in one operator it is ability to set upstream task create kwargs for BatchOperator.partial(...).expand_kwargs(...). But right now BatchOperator can't work with Dynamic Tasks

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Ok, maybe you're right after all. I'll give it a better look. I'm actually not that familiar with batch jobs 😅

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

After taking a closer look at it, I think having 2 (or more) operators would duplicate a lot of code, without removing much complexity.
I see your arguments about how submitting a batch job can mean very different things, but it's also an operation that takes very similar parameters, and for which the actions to take on our side of the API are super similar.

Also, maybe the user isn't always right, but the initial for of this PR comes from an actual user of the operator, so I'd tend to follow their way of thinking (not being a user myself).

@Taragolis

Copy link
Copy Markdown
Contributor

As I mention before I do not not have strict concern about is it should be single operator or 3 operators (Regular, Node, EKS). I use combination Airflow + Batch since Sept 2019, and this combination cover a lot of limitations of each other. Like same implementation as Dynamic Task Mapping available in Batch years ago and work thought arrayProperties, in the other side dependency between Batch Jobs not such good rather Airflow

And there is no many changes happen in Batch operator since this time however the design of Hooks and BatchOperator still from pre-provider era and now it looks ugly even if it have exclusive backoff API caller.

Moth concern that potentially most of the parameters exclusively for containerOverrides options and we do not check it right now. I'm not a user of nodeOverrides because usually such architecture more suits for Hadoop Cluster, so for that purpose better to use EMR. Different users different point.

I would try to check that options and return back after weekend.

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

did you have time to check the options ?
If we want to do it, I think that rewriting the whole batch hook and operator(s) should probably separated from that PR, which is just about resolving a user's issue.

@Taragolis

Copy link
Copy Markdown
Contributor

Sorry, not yet. Hectic days. I will try tomorrow morning.

I we also have a question about log links, but let me check it first.

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me add a bit more context what going on here initially.
Because everything executed outside of the Airflow users do not have any information about logs in AWS Batch.

For regular batch job we have 0 or 1 dict information about Cloudwatch: log group, region name, log stream.
This information mainly for generate operator extra link which is visible int the UI

Grid View
image

Graph View
image

Right now 0 could be in different situations:

  1. User doesn't use Cloudwatch
  2. This is Array Job
  3. For some reason AWS API do not return Cloudwatch link, I personally do not have this situation, but potentially this could happen if JOB finished very quick. That also the reason why we check this in the end of operator execution.

If user use nodeProperties, than jobs would run in multiple places and there is 0..many, in this case we cant utilise Operator Extra Link, so better we could do here is print all links to cloudwatch in Airflow log, but with current implementation the only one would be returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

@vandonr-amzvandonr-amzMar 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok I see your point, but should there really be more than one log link ?
I'm looking at it, and it seems that in the case of a multinode job, there is multiple log_configuration (one per node), but from that log config we get

  • the log group
  • the region

I'd imagine that multinode batch jobs would not be multi-region ? So that'd would be a constant across all nodes.
And also, I suppose in an overwhelming majority of the cases, the log group would be the same for all nodes (it would be very weird if it wasn't).

Then we get the stream name from the attempts, but this does not depend on the number of nodes. I imagine in most cases there would be one attempt. If there are more, we make the choice of returning the stream name for the last attempt, which makes sense.

The job runs on many nodes, but the logs all end up in the same log stream.

What we can do is iterate on the log configs to make sure they are all sending logs

  • to aws
  • in the same region
  • in the same group

and log a warning if it's not the case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd imagine that multinode batch jobs would not be multi-region ?

AFAIK, Batch resources are resource specific for any type of jobs

  • Compute Environment (ECS or EKS clusters)
  • Job Definition
  • Job Queues

You could configure logging to another region (Cloudwatch) or supported logger drivers. But it configure during creation (register) Batch Job Definition and it couldn't change by submit job. So it should all store in one destination

The job runs on many nodes, but the logs all end up in the same log stream.

Nope, each node has own logs within unique log stream

batch-multinode-jobs.mp4

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

wow ok that's hmm... surprising.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

added a commit to log links to all logs

Comment on lines 421 to +422
"""
job_container_desc = self.get_job_description(job_id=job_id).get("container", {})
log_configuration = job_container_desc.get("logConfiguration", {})
job_desc = self.get_job_description(job_id=job_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vandonr-amz can you please add this context to the PR description ^^^ It would be great to have future users able to immediately understand what's going on here.

Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/hooks/batch_client.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py Outdated
Comment threadairflow/providers/amazon/aws/operators/batch.py
@Taragolis

Copy link
Copy Markdown
Contributor

I've tested on own AWS Account this simple DAG

importpendulumfromairflowimportDAGfromairflow.providers.amazon.aws.operators.batchimportBatchOperatorJOB_NAME="multi-node-sample"JOB_DEFINITION="batch-nodes"JOB_QUEUE="multinode-job-queue"CONTAINER_OVERRIDES=NoneARRAY_PROPERTIES=NoneNODE_OVERRIDES= {
"numNodes": 5
}
withDAG(
dag_id="example_batch_submit_job_multi_node",
schedule_interval=None,
start_date=pendulum.datetime(2023, 1, 1, tz="UTC"),
tags=["example", "amazon-provider", "batch", "multi-node"],
catchup=False,
):
submit_batch_job=BatchOperator(
task_id="submit_batch_job",
job_name=JOB_NAME,
job_queue=JOB_QUEUE,
job_definition=JOB_DEFINITION,
container_overrides=CONTAINER_OVERRIDES,
array_properties=ARRAY_PROPERTIES,
node_overrides=NODE_OVERRIDES,
aws_conn_id=None,
)

If only set NODE_OVERRIDES then it run mostly successfully, time to time one or many nodes fail during run without no reason. But it refers ether miss configuration of multinode environment or some internals of AWS.

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

@potiuk
potiuk requested a review from dimbermanMarch 20, 2023 14:58
@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

If I set CONTAINER_OVERRIDES to any value rather than None (even {}), I've got:

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Container overrides and node overrides are mutually exclusive, only one can be set.

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

If I set ARRAY_PROPERTIES than I've got

botocore.errorfactory.ClientException: An error occurred (ClientException) when calling the SubmitJob operation: Multinode Array Job not supported.

Same story here, boto is telling you that you cannot set NODE_OVERRIDES (which implies a multi-nodes job) and ARRAY_PROPERTIES at the same time, though in a less clear way.

I'm not super familiar with batch jobs, but I think the valid combinations are:

  • container_overrides
  • container_overrides + array_properties
  • node_overrides

I haven't tested the array properties, but container and node overrides both work well when not mixed.

@Taragolis

Copy link
Copy Markdown
Contributor

do you mean while still keeping NODE_OVERRIDES set ? That'd be normal, and the error message explains it. If you want to use containers override, you need to unset the node override.

Yep array_properties only allowed for container_overrides, not for node overrides

I'm not super familiar with batch jobs, but I think the valid combinations are

And also I think all of them would conflicts with Batch jobs on EKS 🤣 Bet lets keep it as surprise of future implementations.
Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

Personally I'm not use Multi-node batch jobs (due to all limitations ) but I guess some users might found it useful in some specific cases

well yes, this comes from #25522 which was opened by a user !

@vandonr-amz

Copy link
Copy Markdown
ContributorAuthor

@dimberman@Taragolis do you think you can take a look at the latest changes and see if it looks OK to you ?

@dimbermandimberman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for addressing the changes @vandonr-amz ! LGTM 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support AWS Batch multinode job types

4 participants

@vandonr-amz@Taragolis@dimberman@camilleanne