Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

seperates task and container override #409

Closed
wants to merge 9 commits into from

Conversation

jeanluciano
Copy link
Contributor

@jeanluciano jeanluciano commented Apr 5, 2024

Sepererates the way we override task level cpu and memory from the prefect container. If no container cpu and/or memory is passed, it uses task level.

Example

Checklist

  • [ x ] References any related issue by including "Closes #" or "Closes ".
    • If no issue exists and your change is not a small fix, please create an issue first.
  • [ x ] Includes tests or only affects documentation.
  • [x ] Passes pre-commit checks.
    • Run pre-commit install && pre-commit run --all locally for formatting and linting.
  • Includes screenshots of documentation updates.
    • Run mkdocs serve view documentation locally.

@jeanluciano jeanluciano marked this pull request as ready for review April 9, 2024 17:13
@jeanluciano jeanluciano requested a review from a team as a code owner April 9, 2024 17:13
@jeanluciano jeanluciano self-assigned this Apr 9, 2024
prefect_aws/workers/ecs_worker.py Outdated Show resolved Hide resolved
prefect_aws/workers/ecs_worker.py Outdated Show resolved Hide resolved
@kevingrismore
Copy link
Contributor

kevingrismore commented Apr 10, 2024

I think it'll be important for us to keep in mind that this'll make it easier for users to accidentally supply values to a single container that are larger than the values supplied to the task, especially when they leave the task-level resource settings empty and then request resources for their container that are larger than the default.

I'm ok with letting AWS raise the error for us in cases where users specify many of their own containers and get the math wrong, but the case above seems like an easy pitfall for those not familiar with ECS.

@jeanluciano Do you think we should check if the Prefect container's resources are larger than the task's resources and reject the run with a nice error? Or maybe add a note on the new settings that they can't exceed the task level resources. Like:
"Only use these variables when your ECS tasks have multiple containers. The total CPU and memory for all of a task's containers cannot exceed the CPU and memory set for the task."

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants