Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend luigi.contrib.ecs module to support feeding additional kwargs to ecs run_task #3083

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

greenlaw
Copy link
Contributor

@greenlaw greenlaw commented Jun 8, 2021

Description

This updates the luigi.contrib.ecs module by providing a new overridable run_task_kwargs property that can be used to feed additional keyword arguments to the boto3 ECS run_task call.

Motivation and Context

The existing module only allows users to provide dynamic container commands via containerOverrides, so is very limited to running simple ECS tasks. With these changes, it's now possible to change the launch type on the fly (e.g. EC2 -> FARGATE), give a task additional ephemeral storage, specify custom network configuration, etc.

These changes play nicely with the old version of the module and don't break the previous API.

Have you tested this? If so, how?

Yes, I updated test/contrib/ecs_test.py and all tests pass.

@greenlaw greenlaw requested review from dlstadther, Tarrasch and a team as code owners June 8, 2021 13:05
Comment on lines 179 to 184
if self.command:
if 'containerOverrides' in overrides:
new_commands = []
for command in self.command:
for container_override in overrides['containerOverrides']:
if container_override['name'] == command['name']:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is some deep nesting and is a little hard to follow the logic. Is there a way this could be cleaned up or visually simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will take a look at it this afternoon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a new commit that hopefully makes it clearer, let me know what you think.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.

@stale stale bot added the wontfix label Jan 9, 2022
@greenlaw
Copy link
Contributor Author

Has this project given up on accepting community PRs?

@stale stale bot removed the wontfix label Jan 10, 2022
@dlstadther
Copy link
Collaborator

@greenlaw , to my knowledge this project is still "alive", although very quiet. While I have write access to the repo as a community maintainer, I am not an admin nor employee of Spotify. I cannot speak to Spotify's stance on the luigi project. And while I do have write access, my project context and time grows stale as I no longer utilize luigi in my day job. As such, I have limited time to review and provide feedback here.

As folks submit PRs, I'm happy to take a look. However, I cannot be the sole person reviewing all submissions. Nor can I commit to an SLA on my responses. Please feel welcome to review and provide feedback to other community submissions as your time and energy allows!


For your PR, I apologize for approving your PR so long ago without merging. I've updated the branch and will merge once the checks pass.

@dlstadther dlstadther merged commit 569b00f into spotify:master Jan 11, 2022
@greenlaw
Copy link
Contributor Author

@dlstadther Thank you for the response and merging. I certainly understand constraints on time and appreciate your efforts to maintain this project, apparently by yourself. I was more just generally curious on the state of Luigi, as development velocity does seem to have died down a bit. Perhaps people are focusing their attention on other workflow engines like Argo, Prefect, Airflow, etc. I know that even though we have had success with Luigi, my team has been stretching the limits of what it was intended to do, and will likely be moving away from it for future projects.

@greenlaw greenlaw deleted the ecs-task-kwargs branch January 11, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants