You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/14 23:27:38 UTC
[GitHub] [airflow] o-nikolas opened a new pull request, #27067: Remove deprecated dag_file_processor_timeouts metric
o-nikolas opened a new pull request, #27067:
URL: https://github.com/apache/airflow/pull/27067
This metric was deprecated in 2.0, removed from the documentation, users were notified in changelog, but the metric itself was still in the code base.
fixes #15834
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happened.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the camp of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very litttle we can do about it. Realisation of that is pretty mind-boggling and ot made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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] o-nikolas commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1012402382
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
- # TODO: Remove after Airflow 2.0
Review Comment:
I don't mind adding it, but it's an interesting question. From the (philosophical) discussion above, it seems like we're going to try very hard to not release a major/breaking version for quite some time, so I'm not sure we should leave TODOs for 3.0 yet (or ever?). WDYT from that perspective? also CC @potiuk
--
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] dstandish commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1016149583
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
all suggestions seems a bit confusing.
maybe we need to adopt a policy and say `deprecated and subject to removal per core deprecation policy` or something...
or, short of that.... maybe just
```
Deprecated; will be removed in a future Airflow release.
```
If we don't know _when_ it will be removed, then there's not much value in saying anything about when (i.e. "when we decide to" or "if we ever have another major release".
Or could say
```
Deprecated; may be removed in a future Airflow release.
```
i.e. may instead of will.
But, really that's what deprecated means -- it means it _will_ be removed, so stop using it, no? so let's just call it like it is.
--
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 #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1318943616
Ah no - conflicts
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1015984337
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
- # TODO: Remove after Airflow 2.0
Review Comment:
I proposed neutral wording.
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happened.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the camp of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning for me is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very litttle we can do about it. Realisation of that is pretty mind-boggling and ot made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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 #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1291289263
Let's document it then.
--
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 a diff in pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1011332730
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
- # TODO: Remove after Airflow 2.0
Review Comment:
Can we TODO rmove on 3.0?
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
- # TODO: Remove after Airflow 2.0
Review Comment:
Can we TODO remove on 3.0?
--
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] o-nikolas commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1023280146
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
@potiuk
> is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
Yupp, and creating a dev email list discussion seems like a reasonable next step to me. Perhaps the approach we decide on should even be enshrined in an AIP (even though that might be unconventional).
Ultimately, the most important things to me are to 1) make a group/community decision on the direction (whether that's strict SemVer, our own approach, something in between) 2) document that direction as explicitly and detailed as possible, then 3) stay consistent with whatever we decide.
2 and 3 combined will reduce developer overhead and long discussions each time this question comes up (since it will always come up a lot).
--
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] o-nikolas commented on pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1282802092
> Unfortunately, the code wins, so we can't remove this until 3.0 now. We should instead redocument it as deprecated.
Huh, interesting, I feel like you'd continually collect a basket of things that get dragged through many major versions because we keep forgetting to remove them in X.0.0. The cruelty of SemVer I suppose!
I'll change this PR to just update the docs then. Do I need to update any change log items as well @jedcunningham? Or is that done during the release process?
Also one other general question, do we use any mechanism to track all the changes that need to be released before the next major version? If so, I should update that thing with this change.
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1015960793
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
```suggestion
Stats.incr('dag_processing.processor_timeouts')
# Deprecated. Remove when we decide to remove deprecations
```
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1015997868
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I think we might arrive at some point that we remove some deprecations even if we are "semVer" compliant :). We do it for example when we have security fixes and generally good reason for 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 merged pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
jedcunningham merged PR #27067:
URL: https://github.com/apache/airflow/pull/27067
--
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 #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1291288588
> So IMHO, we need to make a call whether the goal should be to ultimately remove this code, or decide to keep it long term for backcompat but in that case I'd actually like to see it added back to our docs as deprecated but while still describing what it is and what it does, instead of ignoring its existence.
That's fine. I have no **strong** opinion here. I thing you are right that in this case metrics is actually visible, so not explaining it is probably not as good as not explaining class that is actually not visible.
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1016595803
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
Yes. Very much agree with the last proposal from @dstandish . There is no particular need to define in which exact it will be removed. We have no "crystal ball" to tell us the future. Whatever we write there might be a lie. I prefer to be vague rather than potentialy lie. It might well be that at some point in time we relax a bit our approach to backwards compatibiity and we decide that we will allow some, rarely used features to be removed.
Context:
Backwards compatibility and deprecation removal and SemVer might seem like 0/1 game but it isn't. Not even close. Whether something is classified as backwards incomatibility / major version bump (in terms of SemVer versioning) or not highly depends on many factors but most of all it should be a decision made by release manager (based on consensus of the community/PMC members). The code does not decide for us. For example we might decide to disable some feature (we did in the past) when there is a security vulnerabiliyt. Does it break someone's workkflow? Probably yes. SHOULD it break someone workflow ? Probably YES. Is it backwards compatible? Technically yes. Should we bump major version of Airfow to 3 when we do it ? Definitely NOT.
For me backwards compatibility is more close to quantum phusics than binary choices. It's all about probabilty that we will break majority of our user's workflows. Every single release of ours (whether major, minor or patchlevel) breaks someone workflow and this is absolutely inevitable.
This is very well captured in Hyrum's Law: https://www.hyrumslaw.com/ and in the obligatory XKCD:
> With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.
IMHO - what we treat as "backwards incompatible" change should be solely based on our assesment on how big impact it has on our user's migration and whether it is easy to recover for them. It should not be solely based on the actual 'removal" of some feature or changing the defaults. I think this is a very narrow-scoped approach that is not even correct assesment in the first place. You WILL break other's workflows with every release. It's inevitable. Pretending that it's different is just not coping well with reality (which is very far from 0/1 binary choices).
After quite some time of thinking on how we can approach deprecations, I start to think more and more that we should be slightly more liberal when it comes to some deprecations and remove them earlier. The goal of adopting SemVer was not the "versioning" strictness. The main reason we adopted it was because we wanted to make our users comfortable in knowing that their migration will be - very likely - a painless or very easy to handle - experience. We cannot guarantee painless (because of Hyrum's Law). But we can make it super-easy to handle. Goal achieved.
We just need to understand the consequences and deliberately decide, rather than blindly rely on "removed/changed" feature. Blind 0/1 decisions based on code added/removed is goting an easy (and wrong IMHO) path. Nothing frees us from thinking and making decisions. Community over the code is the ASF motto. If we put everything as fully automated rule, we do not need PMC and community any more. Those are guidelines for us, but we should have final say as humans how we interpret those.
INHO - if we do such removal infrequently and we have an easy (possibly even automated or semi-automated or even super-easy to follow by our users) ways how to deal with those, I think we should be able to remove some of the deprecated code without actually breaking our SamVer promises. Something that we will likely be discussing in the future. And I will advocate for that in the future rather than going to Airflow 3.
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happened.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the campl of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very litttle we can do about it. Realisation of that is pretty mind-boggling and ot made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happen.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the campl of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very litttle we can do about it. Realisation of that is pretty mind-boggling and ot made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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 pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1281445464
Unfortunately, the code wins, so we can't remove this until 3.0 now. We should instead redocument it as deprecated.
--
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 #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1288237635
> The cruelty of SemVer I suppose!
I think this is just the price to pay to make our users confident they can upgrade without worrying about incompatibilities. This is the main reason we adopted SemVer, so that we can convinvce our users, that they should migrate early and often without a fear of doing so. I think (based on our survey - https://airflow.apache.org/blog/airflow-survey-2022/ ) we mostly succeeded as the distribution of Airlfow versions is getting very much to what I expected to be (very similar to Python version distribution - with the very same reason behind I think), where users are migrating to newer versions way faster than they did in Airlfow 1.10. I believe adopting SemVer and spreading the word about 'safer" migrations play important part in it (on top of adding really cool features of course). Of course with Providers introducing their own SemVer it makes it more difficult to migrate sometimes, but this is one more reason to migrate often and early - becasue you can address provider incompati
blities one-by-one. This is the "story" we keep on telling our users and using SemVer is part of that "story".
I think as long as "cruelty" does not make a havy toll on maintianers to keep the backwards compatibility, this is not cruel really. Also I think we should figure ways where maintinag compatibility is done in the way that it does not have any impact on maintainability ( I think #26179 is a very good example of that - where the way we provide the compatibilty hsa prety much zero overhead on maintenance). Whenever possible I think we should find a way to keep compatibility in the way that has similar zero-overhead and does not make further development problematic. I see that as much better approach than removing deprecations lightly. And quite often it is a one-time effort to figure out how to do it without leaving a lot of maintainance effort to keep it (for example we can move deprecated code to separate packages/folders where it can be all-but-forgotten).
Only when we have a very good reason to break compatibility and huge maintenance effort, we should do it IMHO. This should be a very deliberate decision and alternatives should be considered before we knowingly break compatibility.
> do we use any mechanism to track all the changes that need to be released before the next major version?
There was an attempt to do it for K8S https://github.com/apache/airflow/pull/26848/files#diff-04b8bc56eb54d028990da2c4cab3101eb2a4aec4f6c8aad34d5230272b5f9874R335 l by @dstandish and add tests that will fail when deprecations are still there.
This would be a nice contribution to track those cases we have already and add tests for it in similar way (possibly even figure out how to do it with minimum effort as a parameterized set of tests.
However I think it makes little sense until we actually agree that there is an Airflow 3 **at all**. So far we have not discussed neither whether at allo, and when we will even attempt to release Airflow 3.
I personally think, unless we have very good reason we should not release Airlfow 3 unless we want to make some marketing around it and it has to be very well thought - why we are doing it and what are the consequences of it. This is pretty much the same approach as Python maintainers have - no-one even attemps to discuss Python 4. And after the Python 2.7 -> Python 3 drama, there must be very good reasons to even start discussing it.
Also one more point -- getting to the subject of this PR.
I think the way it is currently done is a **very good one**. I personally think, there is no reason to document deprecated parameters, classes etc. This is precisely what we've done in #26179 - while the deprecated classes continue to work via dynamic attrs, you do not see them any more. This is one of the best ways you can handle deprecations - if someone used it in the past, they will continue to work, but lack of documentation on them and lack of "easy way' of using it will make new users ... not use them. While we keep compatibility. This is precisely the behaviour we want to induce:
* make 'old users" continue without doing changes if they want (but continue to be annoyed by warnings)
* make "new users" not even aware that there is a deprecated parameter they can use
I think it is good as it is 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] o-nikolas commented on pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1289736364
> > The cruelty of SemVer I suppose!
>
> I think this is just the price to pay to make our users confident they can upgrade without worrying about incompatibilities. This is the main reason we adopted SemVer, so that we can convinvce our users, that they should migrate early and often without a fear of doing so. I think (based on our survey - https://airflow.apache.org/blog/airflow-survey-2022/ ) we mostly succeeded as the distribution of Airlfow versions is getting very much to what I expected to be (very similar to Python version distribution - with the very same reason behind I think), where users are migrating to newer versions way faster than they did in Airlfow 1.10. I believe adopting SemVer and spreading the word about 'safer" migrations play important part in it (on top of adding really cool features of course). Of course with Providers introducing their own SemVer it makes it more difficult to migrate sometimes, but this is one more reason to migrate often and early - becasue you can address provider incompat
iblities one-by-one. This is the "story" we keep on telling our users and using SemVer is part of that "story".
It was a bit of a throw-away comment on my end (which often doesn't translate well over the internet :laughing: ), so that is my bad. But I am actually a huge proponent of SemVer. As a developer, knowing that I can upgrade to any minor version without compatibility issues is an almost unmeasurably great thing.
>
> I think as long as "cruelty" does not make a havy toll on maintianers to keep the backwards compatibility, this is not cruel really. Also I think we should figure ways where maintinag compatibility is done in the way that it does not have any impact on maintainability ( I think #26179 is a very good example of that - where the way we provide the compatibilty hsa prety much zero overhead on maintenance). Whenever possible I think we should find a way to keep compatibility in the way that has similar zero-overhead and does not make further development problematic. I see that as much better approach than removing deprecations lightly. And quite often it is a one-time effort to figure out how to do it without leaving a lot of maintainance effort to keep it (for example we can move deprecated code to separate packages/folders where it can be all-but-forgotten).
I think this is a big one, the more legacy code you pull forward the more complicated your code becomes to maintain and test. Optimizations that reduce overhead like #26179 are not always possible, but we should definitely prioritize those if they are. But also, IMHO, we shouldn't be afraid of major upgrades if and when it becomes valuable to do so (to be clear though, I'm not saying that time is now, or even close to now).
> Also one more point -- getting to the subject of this PR.
>
> I think the way it is currently done is a **very good one**. I personally think, there is no reason to document deprecated parameters, classes etc. This is precisely what we've done in #26179 - while the deprecated classes continue to work via dynamic attrs, you do not see them any more. This is one of the best ways you can handle deprecations - if someone used it in the past, they will continue to work, but lack of documentation on them and lack of "easy way' of using it will make new users ... not use them. While we keep compatibility. This is precisely the behaviour we want to induce:
>
> * make 'old users" continue without doing changes if they want (but continue to be annoyed by warnings)
>
> * make "new users" not even aware that there is a deprecated parameter they can use
>
>
> I think it is good as it is now.
I think there is a risk to incrementing the amount of un-documented/hidden behaviour and it is a double edged sword. It does mean you can get away with fewer major upgrades but also users will ultimately run into these old bits of functionality (by accident or by choice) and it can be confusing for them to navigate from there forward if things like documentation are wiped away. To me this is a bit like security through obscurity.
In this particular case, we now have a metric that is being emitted to users and they 1) shouldn't use it and 2) have no documentation for it if they do notice it or start using it.
So IMHO, we need to make a call whether the goal should be to ultimately remove this code, or decide to keep it long term for backcompat but in that case I'd actually like to see it added back to our docs as deprecated but while still describing what it is and what it does, instead of ignoring its existence.
--
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] o-nikolas commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1015995590
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I like a message like this, more informational rather than a TODO with a defined time. What about this:
```
Deprecated. If and when another major version of Airflow is released, this should be removed.
See https://github.com/apache/airflow/pull/27067#issuecomment-1289736364
```
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I like a message like this, more informational rather than a TODO with a defined time. What about this?:
```
Deprecated. If and when another major version of Airflow is released, this should be removed.
See https://github.com/apache/airflow/pull/27067#issuecomment-1289736364
```
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding.
Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happen.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the campl of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very litttle we can do about it. Realisation of that is pretty mind-boggling and ot made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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] o-nikolas commented on pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1320450102
> Ah no - conflicts
@potiuk I've rebased the branch, sorry for the delay, I had to patch #27770 first because it was blocking the pre-commit for this branch of mine.
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1025463663
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
- # TODO: Remove after Airflow 2.0
Review Comment:
Ok. I will merge as it is for now to not keep it waiting. It's good as it is 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] o-nikolas commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1016005413
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
Hmm, I'm very curious about that! As a developer that's a bit of a trust-buster to have user facing API break in a non-major release. But that's a philosophical discussion for another time :stuck_out_tongue_closed_eyes:
Let me take another stab at something more generic:
```
Deprecated. If and when another major version of Airflow is released, or when
deprecated code is being otherwise culled, this should be removed.
See https://github.com/apache/airflow/pull/27067#issuecomment-1289736364
```
--
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 pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1284284831
> The cruelty of SemVer I suppose!
Indeed.
> Do I need to update any change log items as well
Yeah, please "fix" (remove?) it in the release notes. Might be worth adding another entry via newsfragments that it's deprecated (still).
> do we use any mechanism to track all the changes that need to be released before the next major version?
Nothing formal. Sometimes we add a TODO comment, and thats probably the best option here as we don't actually emit a deprecationwarning.
--
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] o-nikolas commented on a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1017199736
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
Great discussion everyone! Two followups:
1. Related to @dstandish feedback:
> If we don't know when it will be removed, then there's not much value in saying anything about when (i.e. "when we decide to" or "if we ever have another major release".
The value I'm trying to provided with my proposals isn't to forecast _when_ it will be removed, rather to provide some helpful context, namely some examples of conditions that would trigger it's removal. Simply saying "Deprecated; may be removed in a future Airflow release." is a tautology (as anything deprecated is subject to removal at some point in a future release) and doesn't provide the reader any further context beyond the first word "Deprecated". But as always, if I'm out voted, I'm more than happy to go with your suggestion :relaxed:
2. Related to @potiuk feedback:
I'm struggling to find where to land here. I've been part of some discussions where I've suggested deprecating early ("slightly more liberal" as you put it) and I've received push back, and in this thread we're going the other way (at all costs don't deprecate, just keep the behaviour and document it's existence as deprecated). This quantum approach means that no one ever quite knows if and when anything can be removed, so it leads to long discussions like this one. Whereas, if you take SemVer very literally then at least everyone knows, more clearly, how to proceed.
If we're choosing to be more flexible and liberal (which I mostly agree with), then I actually re-propose to remove this metric entirely. It was publicly messaged to be deprecated along with a batch of other metrics, all of which were removed but this one was forgotten; you can really think of this change as a bug-fix of sorts (so no major release needed). If we do the math of how many users will really be affected, versus the code quality improvements of not having to maintain and document this behaviour, I think we could easily land on the removal side. Thoughts?
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happened.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the camp of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning for me is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very little we can do about it. Realisation of that is pretty mind-boggling and it made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus on the above - is to propose to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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 a diff in pull request #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27067:
URL: https://github.com/apache/airflow/pull/27067#discussion_r1020614511
##########
airflow/dag_processing/manager.py:
##########
@@ -1129,7 +1129,6 @@ def _kill_timed_out_processors(self):
)
Stats.decr('dag_processing.processes')
Stats.incr('dag_processing.processor_timeouts')
Review Comment:
I simply try to think and analyse and adapt my understanding of the world. I used to think "backwards compatibillity is easy" and SemVer provides answers. But this was "past". Now we are in the "current".
I've changed my mind seeing some real-world effect and Hyrum's Law is simply reflecting my current understanding. Maybe that's why @o-nikolas some past discussions and push-back of mine migh have mislead you. The push back was based on my (then) strict interpretation of what "backwards incompatibility" is. But since then - things happened.
That includes a user who complained tha their deployment started to throw OOM. Since they had not changed anything between the upgrades, the user questioned backwards compatibility of Airlfow. True story.
For me that was a bit of revelation and true embodiment of Hyrum's Law and the XKCD comic I quoted above. And while I am pretty much still in the camp of using SemVer as communication tool, I am also more on adding more freedom in interpreting of what "backwards compatibility" is.
This has been mostly summarized in my "lazy consensus" post from today about providers: https://lists.apache.org/thread/nvzdgshwm7s0c2sr4sbokp8kbfjnc039. Not later than 6 months ago when we released 2.2+ providers I was an advocate of "breaking change" and "bump major versions" of all providers. Now - with 6 more months of experience, listening to users and looking at other projects (and thinking hard about it) I changed my opinion: the changes in min-airflow-version are unlikely to break someone's workflow, so let's not treat them as breaking. Minor version upgrade is enough.
The most important learning for me is that there is no "objective" criterium for breaking/non-breaking. Something that is not breaking for me, might be breaking for others. And there is very little we can do about it. Realisation of that is pretty mind-boggling and it made me change my mind when it comes to "breaking" interpretation.
This issue here is siimilar:
* Is SemVer still valid for us ? I think yes.
* But - can we remove this metrics without bumping major version? I think yes.
While those two seem contradicting at first, they are not (in my head). While I think breaking changes should bump major versions, such removal is unlikely to break someone workflow, so it is not breaking.
Bottomline:
Yes I think we should remove this metrics now. That would be my vote now.
Proposal:
Now, how we can do some actionable steps from that statement ?
I think there is a way. We could raise a thread on devlist to see if that interpretation of "breaking change" is something that the community is ok with. I think we've never agreed on "what breaking change is". And while it seems obvious - it is not. What's breaking for me might not be necessarily breaking for you.
We could ask on the devlist whether the community is ok with "relaxed" interpretation of "breaking change" - not every removal is breaking, not every parameter change is breaking. Even if technically there are some workflows that will be broken. If we assess that a change is not likely to break many user's workflow - we shoudl be able to treat removals like that as non-breaking.
While this seems like a nuance and unnecessary discussion, I think it is an important decision if we can make it. This will mean that some of the old stuff that we are pretty sure will not wreak havoc with our customers, can be removed without going to Airlfow 3 - while still keeping SemVer promises (interpreted SemVer, yes, but still showing that we promise "easy migration" for all Airflow 2 versions). Eventually it might mean that we will never have Airflow 3 (which I think is not going to happen anyway). And that we will gradually get rid of the deprecations in Airflow 2 rather than cumulating them.
Another option - if we will not be able to get to consensus - is to get rid of the strict SemVer compliance and propose our own non-fully-semver compatible versioning with some more relaxed meaning of "breaking".
I am willing to start such a thread in the next few days. I see in a way what happens with the 2.3+ providers as a test for it. This is an "interpretation" of the SemVer for providers - very similar to the above line of thoughts. If people will be ok with the "non-breaking" 2.3+ provider change, it's a good sign that we can likely apply more relaxed approach for Airflow core as well.
@o-nikolas, @dstandish - is that clearer now where I am coming from ? Is that something that we can agree on as a direction :) ?
--
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 #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1306314648
Proposed neutral message.
--
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 #27067: Document deprecation dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1323376536
Discussion about potential approach we might take for similar cases started in devlist: https://lists.apache.org/thread/1by8ko8jrrp1xwxt5781bwn2tokxjodl
--
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] o-nikolas commented on pull request #27067: Remove deprecated dag_file_processor_timeouts metric
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on PR #27067:
URL: https://github.com/apache/airflow/pull/27067#issuecomment-1279589684
This metric was already communicated to users as being removed in 2.0, so I don't think there's much else to do in this PR but remove the code itself.
Let me know if I'm missing anything else.
--
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