kedro-org / kedro

Kedro is a toolbox for production-ready data science. It uses software engineering best practices to help you create data engineering and data science pipelines that are reproducible, maintainable, and modular.
https://kedro.org
Apache License 2.0
9.91k stars 900 forks source link

Dependabot has been aggressively bumping the lower bound on dependencies #2511

Closed deepyaman closed 1 year ago

deepyaman commented 1 year ago

https://github.com/kedro-org/kedro/pulls?q=is%3Apr+is%3Aclosed+author%3Aapp%2Fdependabot

I've never looked into these until today, but the vast majority of the Dependabot PRs have been unnecessarily restricting the lower bound? Like, just glancing through it, we now require Rich 13.3; every time a minor version comes out, we make that our lower bound?

As an example, any Typer-based plugins may now be incompatible with Kedro, if they use the typer[all] requirement, because they also follow a questionable (Kedro-like :P) policy of restricting the upper bound, and maintainers haven't yet allowed Rich 13.x even though it works.

Just using Rich as an example, but I'm pretty sure the vast majority of these Dependabot PRs over the last 6 months should be undone/refactored...

I think:

Happy to make a PR to make these changes, but I think the Dependabot configuration should also be revisited?

astrojuanlu commented 1 year ago

+100. I think we should be much more permissive, since Kedro is usually installed alongside the other project dependencies, and as such it has to share environment, even if it's not used as a library.

merelcht commented 1 year ago

Hmm actually this is a good point. I thought the ~= notation was more flexible, but reading up on it again I realise some of the bumps have indeed been too restrictive. Is there a way we can loosen it in dependabot?

merelcht commented 1 year ago

FYI on the fsspec pinning, we had to do this to allow for reading of compressed config files: https://github.com/kedro-org/kedro/pull/2375

deepyaman commented 1 year ago

FYI on the fsspec pinning, we had to do this to allow for reading of compressed config files: #2375

@merelcht Sorry, can you point me to a comment on there? :) I see "* Added functionality to the OmegaConfigLoader to load configuration from compressed files of zip or tar format. This feature requires fsspec>=2023.1.0.", but:

  1. Wouldn't that require fsspec>=2023.1.0 as the lower bound on 3.7, too?
  2. Why does it need to be capped fsspec<=2023.1.0 just for 3.7?
merelcht commented 1 year ago
  1. Wouldn't that require fsspec>=2023.1.0 as the lower bound on 3.7, too?

Yes it would, but as I said in here: https://github.com/kedro-org/kedro/pull/2375#discussion_r1126248912 I felt it was a fairly aggressive bound for just one feature that is not likely to be used by many people.

  1. Why does it need to be capped fsspec<=2023.1.0 just for 3.7?

fsspec dropped python 3.7 support after 2023.1.0

deepyaman commented 1 year ago
  1. Wouldn't that require fsspec>=2023.1.0 as the lower bound on 3.7, too?

Yes it would, but as I said in here: #2375 (comment) I felt it was a fairly aggressive bound for just one feature that is not likely to be used by many people.

Thanks for the pointer. I think that's fair, but then I'd assume the same lower bound should be there for Python 3.8 and above, too. It's an aggressive bound still for everybody on 3.8.

  1. Why does it need to be capped fsspec<=2023.1.0 just for 3.7?

fsspec dropped python 3.7 support after 2023.1.0

It's not necessary to separate requirements for this; pip will not try to install a later version if using Python 3.7, because the python_requires on the library for later versions says it must be 3.8 and would be incompatible.