Remove isatty wrapper function
#7814
Conversation
Codecov ReportBase: 89.59% // Head: 89.35% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #7814 +/- ##
==========================================
- Coverage 89.59% 89.35% -0.24%
==========================================
Files 128 128
Lines 15880 15917 +37
Branches 2117 2117
==========================================
- Hits 14228 14223 -5
- Misses 1423 1465 +42
Partials 229 229
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@Kludex I'm currently investigating a problem report from a user running FreeBSD 14.2 and Python 3.11, which sounds pretty much like the problem the original workaround was supposed to prevent: This happens when starting the specific service from a start script on boot when (I assume) stdin is not available. I didn't dig into the issue at hand and also did not look into workarounds, so for now I'm just leaving this comment for others. |
|
Ok, so I can confirm that celery-worker does not start without stdin being available, which used to work until this change. Not sure if it's a big deal, I do have one user though whose setup does not work because of it. This can be worked around outside of celery, but maybe it would make sense to make that part of the code more resilient again (read: not requiring standard input to determine if the terminal supports images). |
|
@auvipy can you revert this? Sorry. |
|
The reason why this change was introduced initially was for Python 2.6, but it worked for other systems. |
|
To test the issue from a shell (close stdin, start worker): |
|
I can try to revert this |
|
Thanks. |
|
It seems a bit strange to me, that this check def supports_images():
return sys.stdin.isatty() and ITERM_PROFILEhappens on stdin (maybe it's because the check should work if either stdout or stderr are redirected?). So instead of reverting the whole patch, only this specific place could be wrapped in a try/except block and return false in case of attribute error (using the logic of the previous function): def supports_images():
try:
return sys.stdin.isatty() and ITERM_PROFILE
except AttributeError:
passI'm not an expert on celery though, so just sharing my thoughts here. Thank you for your quick reaction on my initial comment, much appreciated. |
* PR #7814 reverted * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * format changed * formatted * Update celery/utils/term.py * lint fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * lint fix * tesst added for support images * tesst added for support images * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * spacing fixed --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Asif Saif Uddin <auvipy@gmail.com> Co-authored-by: Omer Katz <omer.katz@kcg.tech>
|
We manually reverted this with suggested improvement and additional tests #9494 . you are most welcome to try this and provide feedback. |
|
@auvipy Can you please summarize what happened in terms of why this change was introduced, and why it was reverted (+minor modification)? |
|
Just noticed this 😂 Unfortunately we’re not there yet haha: |
|
@Nusnus What about just scrolling back on this very issue, e.g., #7814 (comment) In short: The original commit removed a workaround that was thought not to be necessary anymore. You could write:
|
I’d be happy just to scroll up if it would efficiently cross-reference the description with the PR comments of the revert PR and explain concisely what the bottom line was. Overall, when someone reviews the release change log, it is not expected that they will study all of the change items to understand what happened. Providing the community with a concise and clear description is more efficient and will avoid confusion. |
That’s great actually! Thanks! |
|
@Nusnus It used stdin to determine if the terminal supports certain features, but did not take into account, that there sometimes is no stdin for the above mentioned reasons. |
|
The comment #7814 (comment) demonstrates this: So as far as I am concerned, the release note line could be:
|
|
@Nusnus Hopefully the latest version above cuts it (I don't know much about the tests that were added etc. though, as I only reported the original problem) |
|
Hey @grembo - thanks a ton! Really appreciate it 🙏 |
This function was added 11 years ago to avoid an issue with Python 2.6.
References: