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

⬆️ Bulk upgrade common dependencies #450

Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented May 9, 2024

Follows up from #449

Uses find-replace-all to upgrade the following dependencies across pyptoject.toml files:

  • aiohttp
  • bcrypt
  • black
  • pydantic
  • pytest
  • pytest-asyncio
  • pytest-cov
  • pytest-mock
  • pytest-ruff
  • rope
  • ruff

Also specific to redis-events plugin:

  • fastapi
  • redis
  • uvicorn

Please review commit logs to verify this.

Does not include upgrades to older sub-projects, which are pinned to older python versions:

  • kafka_events/kafka_events/v1_0/deliverer/pyproject.toml
  • kafka_events/kafka_events/v1_0/http_kafka_relay/pyproject.toml

Note: I added a bash script that helps with applying poetry lock to every relevant directory. May be helpful for future use.

Tada! 🎉 This should clear up a bunch of open dependabot PRs, and should hopefully make maintaing this multi-project repo a bit easier


Side-note: the following:

  • oid4vci/integration/afj_runner/pyproject.toml

complains about:

The dependency name for controller does not match the actual package's name: acapy-controller

@ff137 ff137 force-pushed the upgrade-common-dependencies branch 2 times, most recently from 4e4f25f to 94065ac Compare May 9, 2024 11:09
@ff137
Copy link
Contributor Author

ff137 commented May 9, 2024

Aah, I've encountered a bug with doing these bulk upgrades.

Integration tests and everything passed on our fork: didx-xyz#191

But then testing with ACA-Py 0.12.1, any method that attempts to use ld_proofs will fail:

2024-05-09 14:48:58,232 aries_cloudagent.core.dispatcher ERROR Handler error: validation_middleware
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.9/site-packages/pyld/context_resolver.py", line 143, in _fetch_context
    remote_doc = jsonld.load_document(url,
  File "/home/aries/.local/lib/python3.9/site-packages/pyld/jsonld.py", line 6583, in load_document

=================
    remote_doc = options['documentLoader'](url, options)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/vc/ld_proofs/document_loader.py", line 111, in __call__
    document = loop.run_until_complete(coroutine)
  File "uvloop/loop.pyx", line 1511, in uvloop.loop.Loop.run_until_complete
  File "uvloop/loop.pyx", line 1504, in uvloop.loop.Loop.run_until_complete
  File "uvloop/loop.pyx", line 1377, in uvloop.loop.Loop.run_forever
  File "uvloop/loop.pyx", line 518, in uvloop.loop.Loop._run
RuntimeError: this event loop is already running.

Some digging revealed to me what's new is that upgrading uvicorn has introduced a new uvloop dependency, which appears to not be compatible with the nest-asyncio dependency used in ACA-Py's ld_proof document loader.

uvicorn was only upgraded for the redis_events plugin. I'll take a look and see what's best way to resolve this - either rolling back, or applying some fix.

@ff137
Copy link
Contributor Author

ff137 commented May 9, 2024

Issue for above-mentioned incompatibility between nest_asyncio and uvloop: openwallet-foundation/acapy#2941

@ff137
Copy link
Contributor Author

ff137 commented May 10, 2024

Managed to fix the above incompatibility by replacing fastapi with fastapi-slim. The latest fastapi now includes all extra dependencies by default, and fastapi-slim is now the one to use for no extras, and no longer includes uvloop

@jamshale
Copy link
Contributor

Thanks for looking into this and doing upgrades. Just a note that now that release workflows are merged, we won't want the aries-cloudagent package to get updated in any lock files. The workflow will do this automatically. It will also attempt to upgrade all dependencies based on the individual configurations, but also common ones will be overridden by plugins_globals.

This is a bit complicated and will make doing upgrades a bit trickier, but hopefully less work to manage a lot of plugins and dependencies. Unfortunately, it caused a lot of merge conflicts. I'm not sure the work involved to rebase this.

@ff137
Copy link
Contributor Author

ff137 commented May 15, 2024

@jamshale that makes sense. I've noticed that installing plugins can downgrade the ACA-Py version, if they're pinned to <0.12 for example. I assume the new workflow helps with that?

And no problem, I'll rebase #449 first, because this PR continued from that one 👌

@ff137 ff137 force-pushed the upgrade-common-dependencies branch from 3d4cca4 to 83f3c90 Compare May 15, 2024 20:09
@jamshale
Copy link
Contributor

Itt should help with that. It updates the lock files for all plugins that pass their tests whenever a new version of aries-cloudagent is published..

I think the downgrade could still happen if you install a plugin, and it's extras, when it is on an older version of aries-cloudagent. Currently all are on 0.12.1 now.

ff137 added 21 commits May 16, 2024 18:29
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
ff137 added 10 commits May 16, 2024 18:31
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 force-pushed the upgrade-common-dependencies branch from 83f3c90 to 9005dc0 Compare May 16, 2024 15:35
@ff137 ff137 marked this pull request as ready for review May 16, 2024 15:36
@ff137
Copy link
Contributor Author

ff137 commented May 16, 2024

Ready for review! I understand there's new workflows in place, for maintaining the acapy version and others with plugins_globals, but idk how that impacts the goal of this PR, of once-off getting everything up to latest. So please let me know any changes that are necessary!

@jamshale
Copy link
Contributor

jamshale commented May 16, 2024

It shouldn't affect this PR. This was done correctly by upgrading the plugin_globals pyproject.toml file and not changing the aries-cloudagent dependency in any lock files.

It would be wrong if it only upgraded dependencies in a plugin that is also shared by the plugin_globals, because the next time the workflow ran it would override the plugins dependency.

Also upgrading aries-cloudagent should only be managed by the release_pr workflow unless the plugin is being repaired after a release in this repo has already occured from aca-py latest.

I'll go over it a bit more carefully soon, but I think we can merge this.

@jamshale jamshale requested review from jamshale, dbluhm and amanji May 16, 2024 18:13
@@ -0,0 +1,18 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost the same as what's found in the repo_manger.py script selection 2 but without overriding with plugin_globals.

Maybe you could add another selection to the repo_manager instead of adding this bash script file?

Some parts of the file are complicated but it should be pretty easy to add this logic to the main method and ignore the rest of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! Didn't notice that python script. That's great, I'll integrate the logic into there instead 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamshale I made the change (we have ChatGPT to thank for the speed). And I included some typo fixes :-)

ff137 added 4 commits May 17, 2024 00:29
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

Great! Thanks for this.

So many spelling mistakes :(

@jamshale jamshale merged commit 03109c3 into openwallet-foundation:main May 16, 2024
3 checks passed
@ff137
Copy link
Contributor Author

ff137 commented May 16, 2024

@jamshale 👌
cSpell vscode extension is the only way I caught them 😄

@ff137 ff137 deleted the upgrade-common-dependencies branch November 6, 2024 17:55
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