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.
>>
>
