Fix setproctitle usage - #53122

Merged
ashb merged 3 commits into
apache:mainfrom
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged

Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom
jason810496:refactor/centralize-setproctitle-utility

Conversation

@jason810496

@jason810496jason810496 commented Jul 10, 2025

Copy link
Copy Markdown
Member

related: #52860

Why

When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.
When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.

What

  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it

@jason810496
jason810496 marked this pull request as ready for review July 10, 2025 10:59
@pierrejeambrun

pierrejeambrun commented Jul 10, 2025

Copy link
Copy Markdown
Member

Might need a second pair of eyes, but LGTM

amoghrajesh
amoghrajesh reviewed Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py Outdated
kaxil
kaxil previously requested changes Jul 10, 2025
View reviewed changes

@kaxilkaxil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers

@jason810496

jason810496 commented Jul 10, 2025

Copy link
Copy Markdown
MemberAuthor

So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?

@kaxil

kaxil commented Jul 10, 2025

Copy link
Copy Markdown
Member

So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?

The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR

@jason810496

jason810496 commented Jul 11, 2025

Copy link
Copy Markdown
MemberAuthor

The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR

I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.
However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).

@jason810496jason810496 changed the title Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496 marked this pull request as draft July 11, 2025 04:40
@jason810496
jason810496force-pushed the refactor/centralize-setproctitle-utility branch from d918926 to 2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496 marked this pull request as ready for review July 14, 2025 09:57
pierrejeambrun
pierrejeambrun reviewed Jul 16, 2025
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.

edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)

Comment threadairflow-core/src/airflow/executors/local_executor.py Outdated
@jason810496
jason810496 marked this pull request as draft July 16, 2025 11:00
@jason810496
jason810496force-pushed the refactor/centralize-setproctitle-utility branch from 2a025af to 7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun previously approved these changes Jul 22, 2025
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there

@jason810496
jason810496 marked this pull request as ready for review July 22, 2025 13:12
@jason810496

jason810496 commented Jul 22, 2025

Copy link
Copy Markdown
MemberAuthor

Looks good. I see the PR is on draft is there anything left you wanted to implement there

No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.

@jason810496
jason810496force-pushed the refactor/centralize-setproctitle-utility branch from 7c758fa to 56af22aCompareJuly 23, 2025 10:58
@jason810496

jason810496 commented Jul 26, 2025

Copy link
Copy Markdown
MemberAuthor

Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!

@jason810496
jason810496force-pushed the refactor/centralize-setproctitle-utility branch from 56af22a to b4786a2CompareAugust 1, 2025 07:13
@jason810496

jason810496 commented Aug 1, 2025

Copy link
Copy Markdown
MemberAuthor

It seems that this PR still needs @kaxil's approval before it can be merged.
(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)

@ashb

ashb commented Aug 1, 2025

Copy link
Copy Markdown
Member

Other committers can dismiss reviews. I've just done that, and let me give it a re-review.

pierrejeambrun
pierrejeambrun reviewed Aug 12, 2025
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested

potiuk
potiuk approved these changes Aug 14, 2025
View reviewed changes

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?

Just a suggestion - not a blocker at all.

@ashb

ashb commented Aug 14, 2025

Copy link
Copy Markdown
Member

@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here

Be Thoughtful about what you add under here

Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.

@ashb
ashb merged commit 30cc87e into apache:mainAug 14, 2025
76 checks passed
@eladkaleladkal mentioned this pull request Sep 5, 2025
Closed
@potiukpotiuk mentioned this pull request Oct 14, 2025
Closed
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees

No one assigned

Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh
, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh

, '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
Fix setproctitle usage - #53122
Merged
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Aug 14, 2025
Merged
Fix setproctitle usage#53122
ashb merged 3 commits into
apache:mainfrom 
jason810496:refactor/centralize-setproctitle-utility
Conversation
@jason810496
@jason810496jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
related: #52860
Why
When using setproctitle we should skip MacOS ( darwin ) as special case, see benoitc/gunicorn#3021 for more detail.

When refactoring in #52860, I found there are duplicate code to handle setproctitle for MacOS and some usage of setproctitle don't handle the MacOS problem, which might lead to error.
What
  • Add a new setproctitle utility in the airflow.utils module to centralize handling of the macOS error, allowing us to avoid the issue from the beginning.
  • Add corresponding tests for it
@jason810496
jason810496
marked this pull request as ready for review
July 10, 2025 10:59
@pierrejeambrun
pierrejeambrun
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
Might need a second pair of eyes, but LGTM
amoghrajesh
amoghrajesh
reviewed
Jul 10, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/setproctitle.py
Outdated

kaxil
kaxil
previously requested changes
Jul 10, 2025
View reviewed changes
@kaxilkaxil
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yeah, like @amoghrajesh mentioned that this is duplicated purposely to not have dependency for it between Core, SDK & Providers
@jason810496
jason810496
commented
Jul 10, 2025

Copy link


Copy Markdown

MemberAuthor
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
@kaxil
kaxil
commented
Jul 10, 2025

Copy link


Copy Markdown

Member
So I need to wait until the PR for code sharing of "core", "task-sdk" and "provider" described in Dev Mail: Code sharing between Airflow Core and Task SDK - how do we achieve it then keep on this right ?
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
@jason810496
jason810496
commented
Jul 11, 2025

Copy link


Copy Markdown

MemberAuthor
The code is too small IMO -- duplication is not always bad :) We can leave this as-is and close this PR
I see! I’ll remove this small new module as I was previously a bit stuck trying to avoid duplication.

However, instead of closing this PR, I’ll update it to fix some remaining setproctitle usages that aren’t properly handled on macOS (e.g. in the Celery executor, edge worker, etc.).
@jason810496jason810496
changed the title
Centralize setproctitle utilityFix setproctitle usageJul 11, 2025
@jason810496
jason810496
marked this pull request as draft
July 11, 2025 04:40
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
d918926 to
2a025afCompareJuly 14, 2025 02:34
@jason810496
jason810496
marked this pull request as ready for review
July 14, 2025 09:57
pierrejeambrun
pierrejeambrun
reviewed
Jul 16, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Just a suggestion, but maybe I missed something.
edit: Just saw other comments, code duplication it is. We can still leave that at the top of each file to avoid duplication within the same file. (which shouldn't bring any cross component dependencies)
Comment threadairflow-core/src/airflow/executors/local_executor.py
Outdated

@jason810496
jason810496
marked this pull request as draft
July 16, 2025 11:00
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
2a025af to
7c758faCompareJuly 20, 2025 12:47
pierrejeambrun
pierrejeambrun
previously approved these changes
Jul 22, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment
•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good. I see the PR is on draft is there anything left you wanted to implement there
@jason810496
jason810496
marked this pull request as ready for review
July 22, 2025 13:12
@jason810496
jason810496
commented
Jul 22, 2025

Copy link


Copy Markdown

MemberAuthor
Looks good. I see the PR is on draft is there anything left you wanted to implement there
No, thanks for the reminder, I just want to make sure the CI is green before marking the PR as ready to review.
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
7c758fa to
56af22aCompareJuly 23, 2025 10:58
@jason810496
jason810496
commented
Jul 26, 2025

Copy link


Copy Markdown

MemberAuthor
Hi @kaxil, this PR still needs your approval to resolve the requested changes. Thanks!
@jason810496
jason810496force-pushed
the
refactor/centralize-setproctitle-utility
branch
from
56af22a to
b4786a2CompareAugust 1, 2025 07:13
@jason810496
jason810496
commented
Aug 1, 2025

Copy link


Copy Markdown

MemberAuthor
It seems that this PR still needs @kaxil's approval before it can be merged.

(Due to GitHub restrictions, if any reviewer has requested changes, the PR requires that specific person's approval to unblock and proceed with the merge.)
@ashb
ashb
commented
Aug 1, 2025

Copy link


Copy Markdown

Member
Other committers can dismiss reviews. I've just done that, and let me give it a re-review.
pierrejeambrun
pierrejeambrun
reviewed
Aug 12, 2025
View reviewed changes
@pierrejeambrunpierrejeambrun
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That looks good to me. I'll let @ashb give the final word on this as requested
potiuk
potiuk
approved these changes
Aug 14, 2025
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

That calls a little for a "shared" distribution to include that code rather than copy it over. But it'a little to small to do it now. But maybe eventually when we expand on our "shared" projects concept we could have a separate "executors base" code an we split out executors to separately installable distributions, we could make the code implemented once. Maybe worth leaving TODO for that ?
Just a suggestion - not a blocker at all.
@ashb
ashb
commented
Aug 14, 2025

Copy link


Copy Markdown

Member
@potiuk I see it differently, and I'd say this is one of the cases where not using a shared dist and duplicating the code is better for readability -- this sort of case is exactly what I had in mind when I wrote this https://github.com/apache/airflow/tree/main/shared#be-thoughtful-about-what-you-add-under-here
Be Thoughtful about what you add under here
Not every piece of code used in two distributions should be automatically placed in one of the shared libraries, and sometimes "just duplicate it" is the right approach to take. For example, if it's just a 5 or 10 line function and it's used in two places, it might be easier for future developers to understand if the function is in two places.
@ashb
ashb
merged commit 30cc87e
into
apache:mainAug 14, 2025

76 checks passed

@eladkaleladkal
mentioned this pull request
Sep 5, 2025

Closed

@potiukpotiuk
mentioned this pull request
Oct 14, 2025

Closed

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

Reviewers

@pierrejeambrunpierrejeambrunpierrejeambrun left review comments
@potiukpotiukpotiuk approved these changes
@kaxilkaxilkaxil approved these changes
@ashbashbAwaiting requested review from ashbashb is a code owner
@mobuchowskimobuchowskiAwaiting requested review from mobuchowskimobuchowski is a code owner
@jscheffljschefflAwaiting requested review from jscheffljscheffl is a code owner
@hussein-awalahussein-awalaAwaiting requested review from hussein-awalahussein-awala is a code owner
@XD-DENGXD-DENGAwaiting requested review from XD-DENGXD-DENG is a code owner
@o-nikolaso-nikolasAwaiting requested review from o-nikolaso-nikolas is a code owner
@amoghrajeshamoghrajeshAwaiting requested review from amoghrajesh

Assignees


No one assigned


Labels

area:CLIarea:Executors-coreLocalExecutor & SequentialExecutorarea:providersarea:task-sdkprovider:celeryprovider:edgeEdge Executor / Worker (AIP-69) / edge3provider:openlineageAIP-53

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jason810496@pierrejeambrun@kaxil@ashb@potiuk@amoghrajesh