Skip to content

(v2) chore(requirements): drop unused dependencies #2060

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

Merged
merged 2 commits into from
Aug 6, 2025

Conversation

noirbee
Copy link
Contributor

@noirbee noirbee commented Jul 2, 2025

These were lifted from the v3 branch wholesale, but are not actually in use in v2.

Changes proposed in this pull request:

  • drop a2wsgi, httpx, uvicorn, typing-extensions direct dependencies
  • typing-extensions still exists as a transitive dependency

These were lifted from the v3 branch wholesale, but are not actually in use in v2.
@noirbee noirbee changed the title chore(requirements): drop unused dependencies (v2) chore(requirements): drop unused dependencies Jul 2, 2025
@potiuk
Copy link

potiuk commented Jul 2, 2025

Nice! Less dependencies is more!

@chrisinmtown
Copy link
Contributor

This is great, thank you.

@chrisinmtown
Copy link
Contributor

@noirbee just out of curiosity, please explain your PR title message convention "(v2) chore(requirements):" I don't see that used heavily in this repo.

@noirbee
Copy link
Contributor Author

noirbee commented Jul 2, 2025

@noirbee just out of curiosity, please explain your PR title message convention "(v2) chore(requirements):" I don't see that used heavily in this repo.

I've added (v2) to the PR title to make it obvious the branch it's targeting in the PR list, but it's just an afterthought, the commit message does not have it.

The kind(scope): message is due to force of habit since it's the convention we use at work

@chrisinmtown
Copy link
Contributor

hi @RobbeSneyders and @Ruwann would you please try to find a moment for this minor improvement in the V2 branch? It passes all checks. Thanks in advance!

@potiuk
Copy link

potiuk commented Jul 8, 2025

I also tested it in Airflow 2.11 - and while I do not have my PR entirely green, I solved most of the test problems and I know the last two issues which I need to solve:

  • we will need to release FAB Provider with support for some dependencies upgraded here
  • MsgSpec serializer (default in new Flask-session 0.7.9+) cannot serialize Markup objects - so flash messages are not serializable - but I will simply switch to json serializer for that.

For all practical purposes - that one could be merged and connextion 2.15.0 can be released with it - we are good with it in Airflow.

@chrisinmtown
Copy link
Contributor

Hey maintainers @Ruwann and @RobbeSneyders I apologize for nagging you, can you review and merge this please?

@chrisinmtown
Copy link
Contributor

Happy July everyone. @RobbeSneyders and @Ruwann please comment - do you want changes here? Is this acceptable? I know we're all volunteers here.

@potiuk
Copy link

potiuk commented Jul 16, 2025

Happy July everyone. @RobbeSneyders and @Ruwann please comment - do you want changes here? Is this acceptable? I know we're all volunteers here.

Yeah, we are also waiting for the release to be able to start working on Airflow 2.11.1 -> we tested this PR, looks good and ife we can get that in, that would help a lot in our efforts to make Airlfow 2 "secure" (at least in the eyes of those who run scans on dependencies of Airflow).

@chrisinmtown
Copy link
Contributor

chrisinmtown commented Jul 17, 2025

@noirbee while you are updating dependencies, you might want to bump the versions too. I ran poetry update locally and it found a few. Here's the main change in poetry.lock, please consider.

 [package.dependencies]
-aiohappyeyeballs = ">=2.3.0"
-aiosignal = ">=1.1.2"
-async-timeout = {version = ">=4.0,<5.0", markers = "python_version < \"3.11\""}
+aiohappyeyeballs = ">=2.5.0"
+aiosignal = ">=1.4.0"
+async-timeout = {version = ">=4.0,<6.0", markers = "python_version < \"3.11\""}
 attrs = ">=17.3.0"
 frozenlist = ">=1.1.1"
 multidict = ">=4.5,<7.0"
-yarl = ">=1.12.0,<2.0"
+propcache = ">=0.2.0"
+yarl = ">=1.17.0,<2.0"

However I see a couple issues here:

  • The new version of yarl adds new dependency propcache. Hopefully this is harmless.
  • pyproject.toml requires PyYAML >= 5.1 so the py39-min minimum-dependency test uses version 5.1. That version has multiple critical vulnerabilities. I believe the min patched version is 5.4. I attempted to use versions 5.4 and 5.4.1, but the py39-min test fails with a complaint about a deprecated license classifier. (Run this command if you want the gory details: pip wheel --no-cache-dir --use-pep517 "pyyaml (==5.4.1)" ). The min test passes on PyYAML version 6.0.2, which is the latest.

Please comment.

@chrisinmtown
Copy link
Contributor

I have one more concern about dependency management. Branch main has an entry for poetry.lock in the .gitignore file. This branch has a committed poetry.lock file and no entry in .gitignore. I think probably you want to follow the example of main and both remove & ignore the poetry.lock file on this branch.

@noirbee
Copy link
Contributor Author

noirbee commented Jul 17, 2025

remove & ignore the poetry.lock file on this branch.

Added 0859743 which does exactly that. I haven't touched / updated the dependencies any further as a result (in pyproject.toml).

@chrisinmtown
Copy link
Contributor

Thanks @noirbee for the quick update to drop poetry.lock I hope the maintainers will comment on how they prefer to manage dependencies and versions. Gosh this waiting, waiting, waiting, WAITING is really starting to drive me a little crazy.

@potiuk
Copy link

potiuk commented Jul 24, 2025

Thanks @noirbee for the quick update to drop poetry.lock I hope the maintainers will comment on how they prefer to manage dependencies and versions. Gosh this waiting, waiting, waiting, WAITING is really starting to drive me a little crazy.

me too :)

@michalmodras
Copy link

Hi @Ruwann and @RobbeSneyders, could you please take a look at this PR & merge if all looks good? This is important to improve Airflow security posture - it would be great to get it merged, so we can make it part of Airflow 2.11. Long-awaited fix in Google Cloud Composer :)
Thank you!

Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the efforts and the patience

@Ruwann Ruwann merged commit 19b72ba into spec-first:v2 Aug 6, 2025
8 checks passed
@chrisinmtown
Copy link
Contributor

Thank you @Ruwann for the merge and new release candidate!

@potiuk
Copy link

potiuk commented Aug 7, 2025

Coool. I am going to try it for Airflow 2 soon :). Thanks for all the efforts everyone

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.

6 participants