Skip to content

Expose device_requests to DockerOperator - Enable GPU support#21974

Closed
TediPapajorgji wants to merge 6 commits into
apache:mainfrom
TediPapajorgji:docker-operator-device-requests
Closed

Expose device_requests to DockerOperator - Enable GPU support#21974
TediPapajorgji wants to merge 6 commits into
apache:mainfrom
TediPapajorgji:docker-operator-device-requests

Conversation

@TediPapajorgji

@TediPapajorgji TediPapajorgji commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

This PR adds the ability to pass device_requests to the DockerOperator during initialization. This is useful for enabling GPU support in the DockerOperator.

This also opens the door to exposing all other host resources to the docker container.

As an example, with this change, here is how one could expose all GPU's on the host machine to the docker container:

train = DockerOperator(
        task_id='<your-id>',
        image='<your-image>',
        api_version='auto',
        auto_remove=True,
        command='<your-command'>,
        device_requests=[
                docker.types.DeviceRequest(count=-1, capabilities=[['gpu']]) # Add all gpu's to the container
            ]
    )

closes: #9492

@boring-cyborg

boring-cyborg Bot commented Mar 3, 2022

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@github-actions github-actions Bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 3, 2022
@github-actions

github-actions Bot commented Mar 3, 2022

Copy link
Copy Markdown
Contributor

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@TediPapajorgji

Copy link
Copy Markdown
Contributor Author

just fixed tests, my apologies

@uranusjr

uranusjr commented Mar 3, 2022

Copy link
Copy Markdown
Member

Please fix the static check failures.

@potiuk

potiuk commented Mar 7, 2022

Copy link
Copy Markdown
Member

Yep. I recommend to install pre-commit.

@TediPapajorgji

Copy link
Copy Markdown
Contributor Author

Yep. I recommend to install pre-commit.

Hey guys i'll do this today.

@eladkal

eladkal commented Mar 14, 2022

Copy link
Copy Markdown
Contributor

@TediPapajorgji you will need also to rebase and resolve conflicts

@TediPapajorgji

Copy link
Copy Markdown
Contributor Author

@eladkal done!

@potiuk

potiuk commented Mar 15, 2022

Copy link
Copy Markdown
Member

Also - the updates to Changelog are not needed (and a bit harmful). The decision of version bump and changelog are generated (semi-automatically) by the release manager when the provider is released so please remove it from this change.

@wcwt

wcwt commented Mar 20, 2022

Copy link
Copy Markdown

Hi Team, How can I update this version of docker?
I saw the update_list is still version 2.5.1
Should it be 2.5.2 after this function is ok?

@eladkal

eladkal commented Mar 20, 2022

Copy link
Copy Markdown
Contributor

@wcwt this PR wasn't merged it.

@TediPapajorgji can you revert changes to CHANGELOG.rst and provider.yml?

@TediPapajorgji

Copy link
Copy Markdown
Contributor Author

@wcwt this PR wasn't merged it.

@TediPapajorgji can you revert changes to CHANGELOG.rst and provider.yml?

Yep I'll do it today.

@eladkal

eladkal commented Mar 20, 2022

Copy link
Copy Markdown
Contributor

Also if possible to add a unit test / add coverage for this change in an existed test.
Maybe

can coverage this change too or create test_execute_with_device_requests?

@potiuk

potiuk commented Mar 29, 2022

Copy link
Copy Markdown
Member

Some conflicts are there as well

@eladkal

eladkal commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

@TediPapajorgji I'd be happy to merge this PR once issues are fixed.
can you address it?

@danycohensa

Copy link
Copy Markdown

@TediPapajorgji could you please check the fail tests? , I really need your changes .
Make my Day !

@eladkal

eladkal commented May 3, 2022

Copy link
Copy Markdown
Contributor

@danycohensa you can checkout @TediPapajorgji branch and re PR the fix (thus it will still give credit to the original author).

@eladkal

eladkal commented May 7, 2022

Copy link
Copy Markdown
Contributor

Closing in favor of #23554

@eladkal eladkal closed this May 7, 2022
@happyhooter

Copy link
Copy Markdown

@eladkal when the next release with updated DockerOperator is planned?

@eladkal

eladkal commented May 16, 2022

Copy link
Copy Markdown
Contributor

@happyhooter RC already cut
#23659
You can help with testing it

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

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DockerOperator support for Nvidia-Docker

7 participants