Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix daskexecutor provider docs #907

Closed
wants to merge 1 commit into from
Closed

Fix daskexecutor provider docs #907

wants to merge 1 commit into from

Conversation

eladkal
Copy link
Contributor

@eladkal eladkal commented Dec 12, 2023

There is no daskexecutor 1.1.1
It seems that doc build created this package though it wasn't part of the release wave - I will investigate and fix the problem with the doc build

@potiuk
Copy link
Member

potiuk commented Dec 12, 2023

Well. We SHOULD release daskexecutor 1.1.1 as part of this wave. That's the whole point of having the last release which will have "removed" notice in the PyPI description - there is no way to change the description in PyPI without releasing the new version of the package.

This is defined as part of the removal process.

@potiuk
Copy link
Member

potiuk commented Dec 12, 2023

So both - releasing 1.1.1 package and documentation for it are absolutely intended and needed.

@eladkal
Copy link
Contributor Author

eladkal commented Dec 12, 2023

Well. We SHOULD release daskexecutor 1.1.1 as part of this wave. That's the whole point of having the last release which will have "removed" notice in the PyPI description - there is no way to change the description in PyPI without releasing the new version of the package.

This is defined as part of the removal process.

No...
we are releasing 1.2.0 because of min airflow version of 2.6
see apache/airflow#36190

@potiuk
Copy link
Member

potiuk commented Dec 12, 2023

Ah then 1.2.0 is wrong. We should not bump Airflow min version for daskexecutor. The "upgrade min airflow" release command will sklp bumping it at least (deliberately)

We should not bump min-airflow version for this package - it should be identical as the last released version with the only difference being the description in PyPI. There is no particular reason why we should bump it for such a package.

@eladkal
Copy link
Contributor Author

eladkal commented Dec 12, 2023

We should not bump min-airflow version for this package - it should be identical as the last released version with the only difference being the description in PyPI. There is no particular reason why we should bump it for such a package.

That means modifying pre-commit and enabling 2.5.0 back
This is an edge case of an edge case that the removal and bump min airflow version are closed to each other.

We should not bump min-airflow version for this package

Why do you feel strongly about it? what is the down side?

@potiuk
Copy link
Member

potiuk commented Dec 12, 2023

That means modifying pre-commit and enabling 2.5.0 back

Does it? I think we do not do any pre-commit that would prevent dask executor to keep 2.5.0 (but maybe I am wrong and we need to FIX a pre-commit in question to allow it for removed providers).

Why do you feel strongly about it? what is the down side?

Because the only reason for releasing the package is that we want the pypi description to change to signal that it has been removed. I do not feel super-strong about it, but at least my vision for it that the package should be exactly the same as before - with the only difference being description. Just to avoid any potential issues, I thought about it "let's not change anything important in the package at all".

Bumping min-version only makes sense when we want to have some future changes there to make people "want" to upgrade to newer version of Airflow. In case of removed provider - this motivation for bumping min-airflow version is gone (because there are no new features, and will not be new features any more) so bumping min-airflow version in this case is nothing else than "cargo cult" - we will do it because we generally do it for other packages, without actual reason for doing it.

I cannot think of particular scenario where it would be problematic, but I think there is simply absolutely no reason do it. And I very much don't like we do anything unless we have a good reason for it.

@eladkal
Copy link
Contributor Author

eladkal commented Dec 12, 2023

Following our discussion closed due to apache/airflow#36191
Thanks @potiuk

@eladkal eladkal closed this Dec 12, 2023
@kaxil kaxil deleted the dask branch August 16, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants