You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ferruzzi (via GitHub)" <gi...@apache.org> on 2023/03/10 17:32:27 UTC
[GitHub] [airflow] ferruzzi opened a new pull request, #30024: Improve type hinting in stats.py
ferruzzi opened a new pull request, #30024:
URL: https://github.com/apache/airflow/pull/30024
Does what it says on the tin. Increases the level of type hinting in stats.py and makes a couple of small adjustments to make mypy happy.
Lead-up to OpenTelemetry AIP-48 support
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1464329663
Nothing jumps out and other PRs are passing which implies it's a "me" problem. running the full CI locally to see if I can reproduce and I'll go from there. If anyone sees something obvious, let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1468626281
LOL Cursed PR. I moved the two imports behind a check as @uranusjr said, that needed to be fixed anyway, and pushed those changes. Not sure that that had to do with the test failing for image build times, but hey, it's progress. Now it's cancelling `Tests / Test Pytest collection (pull_request) Cancelled after 5m ` with the error message `Error: The operation was canceled.` LOL Such an odd PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1468818760
Quick, someone merge while it's green LOL Looks like a combination of flake and that `if TYPE_CHECKING` block did the trick. :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #30024: Improve type hinting in stats.py
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #30024:
URL: https://github.com/apache/airflow/pull/30024
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1466825131
Maybe not? That's three or four consecutive runs it's failed on this PR now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #30024: Improve type hinting in stats.py
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1465283795
We have a flaky test there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #30024: Improve type hinting in stats.py
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #30024:
URL: https://github.com/apache/airflow/pull/30024#discussion_r1160932778
##########
airflow/stats.py:
##########
@@ -159,35 +163,37 @@ class Timer(TimerProtocol):
_start_time: int | None
duration: int | None
- def __init__(self, real_timer=None):
+ def __init__(self, real_timer: Timer | None = None) -> None:
self.real_timer = real_timer
- def __enter__(self):
+ def __enter__(self) -> Timer:
return self.start()
- def __exit__(self, exc_type, exc_value, traceback):
+ def __exit__(self, exc_type, exc_value, traceback) -> None:
self.stop()
- def start(self):
+ def start(self) -> Timer:
"""Start the timer."""
if self.real_timer:
self.real_timer.start()
- self._start_time = time.perf_counter()
+ self._start_time = int(time.perf_counter())
return self
- def stop(self, send=True):
+ def stop(self, send: bool = True) -> None:
"""Stop the timer, and optionally send it to stats backend."""
- self.duration = time.perf_counter() - self._start_time
+ self.duration = int(time.perf_counter()) - (self._start_time or 0)
Review Comment:
This limits all timers to second gruanularity in output -- I don't think we wanted to do this do we?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1464194761
Nah, I have honestly never seen that particular failure. I have to step away for about an hour, but I'll poke it when I get back. I noticed the branch was a couple commits behind so I rebased and pushed again in case it just needed a bump. If it fails this time, I'll see what I can find.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a diff in pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #30024:
URL: https://github.com/apache/airflow/pull/30024#discussion_r1163170894
##########
airflow/stats.py:
##########
@@ -159,35 +163,37 @@ class Timer(TimerProtocol):
_start_time: int | None
duration: int | None
- def __init__(self, real_timer=None):
+ def __init__(self, real_timer: Timer | None = None) -> None:
self.real_timer = real_timer
- def __enter__(self):
+ def __enter__(self) -> Timer:
return self.start()
- def __exit__(self, exc_type, exc_value, traceback):
+ def __exit__(self, exc_type, exc_value, traceback) -> None:
self.stop()
- def start(self):
+ def start(self) -> Timer:
"""Start the timer."""
if self.real_timer:
self.real_timer.start()
- self._start_time = time.perf_counter()
+ self._start_time = int(time.perf_counter())
return self
- def stop(self, send=True):
+ def stop(self, send: bool = True) -> None:
"""Stop the timer, and optionally send it to stats backend."""
- self.duration = time.perf_counter() - self._start_time
+ self.duration = int(time.perf_counter()) - (self._start_time or 0)
Review Comment:
Yeah, what Jed said. That was to make MyPy happy due to the underlying typing for `_start_time`. Thanks for fixing that, I should have questioned it instead of just rolling with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #30024: Improve type hinting in stats.py
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #30024:
URL: https://github.com/apache/airflow/pull/30024#discussion_r1160948144
##########
airflow/stats.py:
##########
@@ -159,35 +163,37 @@ class Timer(TimerProtocol):
_start_time: int | None
duration: int | None
- def __init__(self, real_timer=None):
+ def __init__(self, real_timer: Timer | None = None) -> None:
self.real_timer = real_timer
- def __enter__(self):
+ def __enter__(self) -> Timer:
return self.start()
- def __exit__(self, exc_type, exc_value, traceback):
+ def __exit__(self, exc_type, exc_value, traceback) -> None:
self.stop()
- def start(self):
+ def start(self) -> Timer:
"""Start the timer."""
if self.real_timer:
self.real_timer.start()
- self._start_time = time.perf_counter()
+ self._start_time = int(time.perf_counter())
return self
- def stop(self, send=True):
+ def stop(self, send: bool = True) -> None:
"""Stop the timer, and optionally send it to stats backend."""
- self.duration = time.perf_counter() - self._start_time
+ self.duration = int(time.perf_counter()) - (self._start_time or 0)
Review Comment:
I assume it wasn't intentional, and was driven by the fact `_start_time` was originally typed as int. #30532 changes it back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1464182576
Well that's a new and exciting CI failure. I'll have a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #30024: Improve type hinting in stats.py
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1466494618
Ah, thanks. I was planning to come back at i this morning with a fresher brain. :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #30024: Improve type hinting in stats.py
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1464187731
> Well that's a new and exciting CI failure. I'll have a look
Do i feel some sarcasm here :) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #30024: Improve type hinting in stats.py
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1467336677
Maybe try reducing the functional changes in this PR? I can spot of couple:
1. The imports to `datadog` and `statsd` should be put behind an `if TYPE_CHECKING` guard.
2. There’s a place `get` is changed to `getboolean`; this may cause incompatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #30024: Improve type hinting in stats.py
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30024:
URL: https://github.com/apache/airflow/pull/30024#issuecomment-1468835180
> Quick, someone merge while it's green LOL Looks like a combination of flake and that `if TYPE_CHECKING` block did the trick. 👍
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org