Re-worked the PR - following Ash's suggestion using `ruff graph analyze` -
https://github.com/apache/airflow/pull/44686#issuecomment-2525298095 - I
think it's indeed better, even if that change is even bigger this way - I
had to extract test_utils.compat.py versions to separate
"version_compat.py". I think `version_compat.py` is a good name for such a
file that will get copied to the providers that need them. The nice thing
is that you really have to just copy that one file and import your
constants from there if you want to add version support to the provider -
and the instructions that pre-commit will print in this case are pretty
straightforward.
[image: image.png]
I wondered if we should maybe check if the copied modules are identical
(they should - i.e. whether someone did not modify them - but that would
mean yet another pre-commit on top of the one that runs `ruff analyze
graph` - so I am not sure if it is worth it, if someone modifies it (say
removes unused constants) and the provider still works, that will be
perfectly fine.
J.
On Sat, Dec 7, 2024 at 3:28 PM Jarek Potiuk <ja...@potiuk.com> wrote:
> Also.. For those interested why this "seemingly simple" case is not that
> simple - one more small comment and context.
>
> While you might think it's stupid and simple, there is a reason why we
> "want" to do a hand-holding - or at least guide-railing here. The thing is
> that the simple and obvious:
>
> if Version(_version) >= Version("2.3.0"):
>
> is unfortunately (and non-obviously) wrong.
>
> Simply because we have pre-release versions and Python version PEP set
> some "obvious" and "correct" rules for comparison of versions in PEP 440
> https://peps.python.org/pep-0440/, but they made it useless for very
> needed use-case including pre-release version.
>
> > Within a numeric release (1.0, 2.7.3), the following suffixes are
> permitted and MUST be ordered as shown:
> > .devN, aN, bN, rcN, <no suffix>, .postN
>
> This means that - effectively - 2.3.0rc1 is NOT >= Version("2.3.0") when
> you use sorting order defined in PEP 440. And in case when we want to
> include pre-releases in our comparison whether there is a new feature we
> need to do this double Version snippet of code which is far from obvious.
>
> There is a long recent discussion in `pip` -
> https://github.com/pypa/pip/issues/13089. where I took part and advocated
> for having a feature in `pip` that would help us with making it easier to
> specify package requirements (currently we have to dynamically modify our
> requirements during building in order to overcome this limitation where
> pre-release packages are involved. But I agree with `pip` maintainers, that
> having a possibility of built-in different comparison in "packaging"
> requires really a whole new PEP, then implementation in packaging and only
> then making it usable in `pip` (and our code).
>
> So this is one more reason why we should make it easy and consistent to
> make those "feature availability" comparisons - because when we have 6
> different versions of it, someone will get confused and will start maybe
> using the 7th (Version(_version) >= Version("2.3.0") - which will look
> right, but will be wrong when we attempt to make a release.
>
> So I'd say, it's a real issue to address.
>
> J.
>
>
>
>
>
> On Sat, Dec 7, 2024 at 11:52 AM Jarek Potiuk <ja...@potiuk.com> wrote:
>
>> > Yes we will have to wait some time before all providers can use such a
>> helper, but time flies, and there's no emergency here.
>>
>> I think that's the key. This is not a question of emergency but having
>> confusion, mistakes for the next at least 8 months. I like the
>> https://github.com/apache/airflow/pull/44607 better because it is
>> simpler. And maybe this is a good one as long as we work out strategy how
>> to get there.
>>
>> One could argue that adding such a solution in Airflow will add **MORE**
>> possibilities to make mistakes. What happens if someone starts using this
>> method in their provider between now and 8 months from now? That will -
>> accidentally - make such a provider not work on Airflow 2.8, 2.9, 2.10.
>> Only 2.11 will work (as long as we back-port it to v2-10-test/
>>
>> Luckily, we have the back compatibility tests that **should** catch
>> it, but I can imagine the case that someone adds this if and it will not be
>> covered by tests. Then - having it in airflow will add more possibilities
>> of introducing issues.
>>
>> So ... ideally if we introduce this one, we should also somehow (yet
>> another pre-commit?) that it is **not used** till 8 months pass (or
>> generally till min-airflow-version will be 2.11). If we do it, I am all for
>> it as additional solution - then we will be able to remove whatever we come
>> up with for the next 8 months.
>>
>> J.
>>
>>
>>
>>
>>
>>
>> On Fri, Dec 6, 2024 at 8:48 PM Daniel Standish
>> <daniel.stand...@astronomer.io.invalid> wrote:
>>
>>> There are multiple different issues implicated here but the only one I am
>>> interested in is these version constants. And that's what I'll address
>>> here.
>>>
>>> My take is that adding constants like this in ... all? ... providers is
>>> unnecessary and overkill and very bloaty.
>>>
>>> PROVIDERS_{{ PROVIDER_CONSTANT_PREFIX }}_AIRFLOW_IS_2_9_PLUS
>>>
>>> All we're really after is, "what is the airflow version?"
>>>
>>> The context here is when providers are implementing conditional logic
>>> based
>>> on airflow core version. Let's also remember that there will soon be a
>>> SDK... shall we add constants for that too?
>>>
>>> I think we're better off if we simply provide a simple way to answer the
>>> question, "what is the core version?"
>>>
>>> We can do this in one place instead of many.
>>>
>>> I have offered two ways we could do that.
>>>
>>> One is #44607 <https://github.com/apache/airflow/pull/44607> which adds
>>> a
>>> get version function that gives the base version, which is what all of
>>> these comparisons use.
>>>
>>> Usage example:
>>>
>>> if get_airflow_version() > Version("3.0.0"):
>>> do_something()
>>>
>>> Another, which I think I like better is, #44743
>>> <https://github.com/apache/airflow/pull/44743>, which returns an
>>> AIRFLOW_BASE_VERSION constant that can be compared both with Version
>>> objects and with strings.
>>>
>>> Usage example:
>>>
>>> if AIRFLOW_BASE_VERSION >= "2.10.0":
>>> do_something()
>>>
>>> This way we don't need to add such constants in every provider, for every
>>> version, for both "greater equals" and "greater than".
>>>
>>> You just give the info and the call site does the evaluation.
>>>
>>> Yes we will have to wait some time before all providers can use such a
>>> helper, but time flies, and there's no emergency here.
>>>
>>> The other issue, overwriting the autogen file has now already been fixed,
>>> thanks to Jarek. And the cross provider import thing, I will leave that
>>> for others to debate.
>>>
>>
