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

Rename DAG concurrency settings for easier understanding #16267

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Jun 4, 2021

dag_concurrency -> max_active_tasks_per_dag

ToDo:

  • Add an example scenario where these settings will be useful:
    max_active_tasks_per_dag : "To stop a new dag with an early start date from stealing all the executor slots in a cluster?"
  • Rename DAG.concurrency
  • Add note in UPDATING.md
  • Rename BaseOperator.task_concurrency settings too. (Will do this as a separate PR as the change is becoming quite BIG)

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

This PR is an attempt to make the settings easier to understand


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@kaxil kaxil force-pushed the rename-concurrency branch 3 times, most recently from 1794c4e to 80ee573 Compare June 8, 2021 23:55
@kaxil kaxil closed this Jun 9, 2021
@kaxil kaxil reopened this Jun 9, 2021
@kaxil kaxil closed this Jun 9, 2021
@kaxil kaxil reopened this Jun 9, 2021
@kaxil kaxil closed this Jun 9, 2021
@kaxil kaxil reopened this Jun 9, 2021
@kaxil kaxil force-pushed the rename-concurrency branch 8 times, most recently from 740c91c to 516539c Compare June 16, 2021 13:05
@kaxil kaxil changed the title Rename various concurrency settings to be easier to understand Rename DAG concurrency settings for easier understanding Jun 16, 2021
@kaxil kaxil marked this pull request as ready for review June 16, 2021 13:05
Copy link
Contributor

@ryanahamilton ryanahamilton left a comment

Choose a reason for hiding this comment

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

I appreciate how I very clearly know what this means by just reading the name.

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense now! Thanks Kaxil!

Just some REST API test fix. LGTM

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 16, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

``dag_conccurency`` ->  ``max_active_tasks_per_dag``
kaxil added a commit to astronomer/airflow that referenced this pull request Aug 19, 2021
Follow-up of apache#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)
kaxil added a commit that referenced this pull request Aug 19, 2021
Follow-up of #16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)
kaxil added a commit to astronomer/airflow that referenced this pull request Aug 19, 2021
Follow-up of apache#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand
kaxil added a commit that referenced this pull request Aug 19, 2021
Follow-up of #16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 10, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Mar 10, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 4, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jul 10, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
@jbmeerkat
Copy link

Very clear name max_active_tasks_per_dag became obscured max_active_tis_per_dag somehow. How did it happen? This "tis" is not clear at all 😞

@potiuk
Copy link
Member

potiuk commented Aug 3, 2022

Very clear name max_active_tasks_per_dag became obscured max_active_tis_per_dag somehow. How did it happen? This "tis" is not clear at all disappointed

Your praised very simple max_active_tasks_per_dag was simply wrong name. There is a difference between task and task instance. The tis is often used all over the code for task_instance and this is right. max_activet_tasks_instances_per_dag would be simply too long.

It's all perfectly logical and accurate. As opposed to previous name which was wrong and illogical.

@jbmeerkat
Copy link

Your praised very simple max_active_tasks_per_dag was simply wrong name. There is a difference between task and task instance. The tis is often used all over the code for task_instance and this is right. max_activet_tasks_instances_per_dag would be simply too long.

It's all perfectly logical and accurate. As opposed to previous name which was wrong and illogical.

Jarec thank you very much for clarification. Now I see why that changed!

To be more clear I'm looking not from an Airflow developer perspective but from an Airflow user perspective, so I don't know how it's named in the code. I just updated my Airflow instance and started to see a deprecation warnings about this "tis" 😄

I'm not trying to dispute that name, just telling about my experience.

@potiuk
Copy link
Member

potiuk commented Aug 3, 2022

I'm not trying to dispute that name, just telling about my experience.

And I am just answering your question - very precisely and accurately - "How did it happen?".

@jbmeerkat
Copy link

I'm not trying to dispute that name, just telling about my experience.

And I am just answering your question - very precisely and accurately - "How did it happen?".

Thank you for that!

leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Aug 27, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 4, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
aglipska pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 7, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Dec 7, 2022
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
Follow-up of apache/airflow#16267 . Docstrings should show new values instead of deprecated. (`max_active_tasks` instead of concurency)

GitOrigin-RevId: 3b41bb45e6e25c1482847e30793c2413b386589b
leahecole pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jan 27, 2023
Follow-up of apache/airflow#16267

Renames `task_concurrency` to `max_active_tis_per_dag`

Some of Airflow's concurrency settings have been a source of confusion for a lot of users (including me), for example:

https://stackoverflow.com/questions/56370720/how-to-control-the-parallelism-or-concurrency-of-an-airflow-installation
https://stackoverflow.com/questions/38200666/airflow-parallelism
This PR is an attempt to make the settings easier to understand

GitOrigin-RevId: 35a6c302772bf5d0bb74646b2da04856c7d7aaac
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.

None yet

5 participants