Patch utcnow in retry delay test

[edwardwang888]

This test uses the current timestamp and sleep to test retry_delay. This is nondeterministic and fails on my local machine as it takes longer than 3 seconds to execute the first two runs. The retry_delay code was originally written using datetime.datetime.now, which is a built-in method and probably could not be easily patched without affecting other parts of the code that used datetime.datetime. However, now that timezone.utcnow is used, patching is possible. The logic of timezone.utc does need to be duplicated, though it is only two lines.

airflow/airflow/utils/timezone.py

Lines 53 to 65 in 87e9865

	def utcnow() -> dt.datetime:
	"""
	Get the current date and time in UTC
	:return:
	"""
	# pendulum utcnow() is not used as that sets a TimezoneInfo object
	# instead of a Timezone. This is not picklable and also creates issues
	# when using replace()
	result = dt.datetime.utcnow()
	result = result.replace(tzinfo=utc)
	return result

---

Original commit that added these tests: 8051aa9#diff-969c0712020e8472d8d78ae32190692cc1e8a3e16dc17f6977c8f87a1328adb7R142

At that time, retry_delay logic was done using datetime.datetime.now:

airflow/airflow/models.py

Lines 928 to 934 in 8051aa9

	def ready_for_retry(self):
	"""
	Checks on whether the task instance is in the right state and timeframe
	to be retried.
	"""
	return self.state == State.UP_FOR_RETRY and \
	self.end_date + self.task.retry_delay < datetime.now()

---

^ Add meaningful description above

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

[edwardwang888]

The Py3.6 tests are failing due to tests/kubernetes/test_pod_generator.py, but it looks like tests/models/test_taskinstance.py was successful. So I think it is okay?

[mik-laj]

This library is not an Airflow dependency. If we want to use it for core testing, it should be installed with devel extra.
https://github.com/apache/airflow/blob/main/setup.py#L495
Or maybe you have an idea how to write this test without using this library?

[edwardwang888]

Yeah, it is not strictly necessary. I was just trying to produce the closest boundary case 🙂

In general, is there a preference of whether to add (not strictly necessary) libraries to the devel extra?
https://github.com/apache/airflow/blob/3f65a48bcedfab6a50e24ecac710170aab6f51bc/tests/models/test_taskinstance.py#L526-L532

[edwardwang888]

Actually, I just saw #12500. It doesn't seem necessary to require numpy just for this test, so I will change it. But do you know why the test still passed in the CI environment?

EDIT: It looks like pandas is still a library in devel_ci. I think that's why.

[mik-laj]

We have a lot of transitive dependencies. In this case, we have installed numpy on CI, because. it is required by apache-beam, pandas and pyarrow.

$ pipdeptree -r -p numpy
numpy==1.19.5
  - apache-beam==2.32.0 [requires: numpy>=1.14.3,<1.21.0]
  - pandas==1.1.5 [requires: numpy>=1.15.4]
    - pandas-gbq==0.14.1 [requires: pandas>=0.20.1]
    - scrapbook==0.5.0 [requires: pandas]
  - pyarrow==4.0.1 [requires: numpy>=1.16.6]
    - apache-beam==2.32.0 [requires: pyarrow>=0.15.1,<5.0.0]
    - scrapbook==0.5.0 [requires: pyarrow]

Airflow does not depend on these dependencies, so if we want to use this library, we should add it.

$ curl -s https://raw.githubusercontent.com/apache/airflow/main/setup.{py,cfg} | grep -C 10 numpy | wc -l
0

In general, is there a preference of whether to add (not strictly necessary) libraries to the devel extra?

In this case, we should add this library to devel, because itis needed for core.

extra	command	description
devel	pip install 'apache-airflow[devel]'	Minimum dev tools requirements (without providers)
devel_hadoop	pip install 'apache-airflow[devel_hadoop]'	Same as devel + dependencies for developing the Hadoop stack
devel_all	pip install 'apache-airflow[devel_all]'	Everything needed for development (devel_hadoop + providers)
devel_ci	pip install 'apache-airflow[devel_ci]'	All dependencies required for CI build.

See: http://airflow.apache.org/docs/apache-airflow/stable/extra-packages-ref.html#bundle-extras

[mik-laj]

I also support the idea of getting rid of this dependency, but if you don't find another solution then it's okay.

[edwardwang888]

Thanks for the explanation! Yes, I found a solution where the difference is practically negligible, so I will just change it and remove numpy from the test.

[mik-laj]

What do you think about freezegun?

[edwardwang888]

Cool, looks like exactly what I need!

[potiuk]

The Py3.6 tests are failing due to tests/kubernetes/test_pod_generator.py, but it looks like tests/models/test_taskinstance.py was successful. So I think it is okay?

Fixing it now by refreshing latest constraints. Hopefully next time you rebase it will be gone.

[edwardwang888]

@mik-laj This should be ready now. The tests are still failing but I believe the test I changed is okay.

[edwardwang888]

@mik-laj I have rebased to the latest main. All tests except one are now passing, but the failing test looks unrelated to this change.

[github-actions]

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