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

feat: use uv if installed for ape plugins install / uninstall commands #2000

Merged
merged 34 commits into from
Apr 23, 2024

Conversation

wakamex
Copy link
Contributor

@wakamex wakamex commented Apr 11, 2024

What I did

add setuptools and pip to base requirements so that uv can install everything correctly.

fixes: #1999

fixes: #

How I did it

edited the setup.cfg

How to verify it

uv venv .venv -p 3.10
source .venv/bin/activate
uv pip install -e ../newape
uv pip install -e ../ape-alchemy
ape plugins list
Installed Plugins
alchemy 0.7.1.dev11+gad25ef2.d20240411
ape --version
0.7.14
ape networks list
ethereum (default)
├── goerli
│ ├── alchemy
│ └── geth (default)
├── local (default)
│ ├── geth
│ └── test (default)
├── mainnet
│ ├── alchemy
│ └── geth (default)
└── sepolia
├── alchemy
└── geth (default)
python ../testape/steth.py
alchemy through ape took 1.13 seconds for 1076 events (events per second: 953.32)

Checklist

  • Consider eliminating pip dependency in favor of pkg_resources
  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@wakamex wakamex changed the title make uv installable: add setuptools and pip to base requirements feat: make uv installable: add setuptools and pip to base requirements Apr 11, 2024
fubuloubu
fubuloubu previously approved these changes Apr 11, 2024
setup.py Outdated Show resolved Hide resolved
@fubuloubu fubuloubu requested a review from antazoey April 11, 2024 20:49
setup.py Outdated Show resolved Hide resolved
@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

I think I made uv a stand-in replacement for pip only if it's installed

saw some warnings along the way:

.venv/lib/python3.10/site-packages/eth/__init__.py:1
  /code/newape/.venv/lib/python3.10/site-packages/eth/__init__.py:1: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
    import pkg_resources

.venv/lib/python3.10/site-packages/_pytest/config/__init__.py:1373
  /code/newape/.venv/lib/python3.10/site-packages/_pytest/config/__init__.py:1373: PytestConfigWarning: Unknown config option: timeout
  
    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

@wakamex wakamex force-pushed the add_setuptools_pip_requirements branch from e16e99e to 8368de2 Compare April 15, 2024 01:41
@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

evm-trace has a capped dependency on py-evm versions which use pkg_resources. I remove it here: ApeWorX/evm-trace#62

@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

uncapping python-dateutil, eth-account, eth-utils, web3[tester], and eip712 points to the culprit:

uv pip install -e . --prerelease=allow
   Built file:///code/newape                                                                                          Built 1 editable in 676ms
Updating https://github.com/wakamex/evm-trace.git (HEAD)
⠙ rich==13.7.1                                                                                                          × No solution found when resolving dependencies:
  ╰─▶ Because eip712>=0.2.3 depends on eth-account>=0.10.0,<0.11 and only the following versions of eip712 are
      available:
          eip712<=0.2.3
          eip712==0.2.4
          eip712==0.2.5
      we can conclude that eip712>=0.2.3 depends on eth-account>=0.10.0,<0.11.
      And because eth-ape==0.7.15.dev5+gdbe67d70.d20240415 depends on eth-account>=0.11.2, we can conclude that
      eip712>=0.2.3 and eth-ape==0.7.15.dev5+gdbe67d70.d20240415 are incompatible.
      And because eth-ape==0.7.15.dev5+gdbe67d70.d20240415 depends on eip712>=0.2.3 and you require

uv has really nice dependency errors :)

my guess is eip712 needs to be updated to be uncapped

@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

uncapping and bumping pandas to >=2 lets uv pick 2.2.2 instead of 1.5.3 (why that one? i dunno... probably picking the oldest version that meets criteria?)

1.5.3 results in a hecking weird error:

ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

that version is from Jan 19, 2023 so imma say it's just busted

error is now back to pkg_resources usage in eth_tester

@wakamex wakamex force-pushed the add_setuptools_pip_requirements branch from cd8cff3 to 8c4ad95 Compare April 15, 2024 05:56
@wakamex wakamex closed this Apr 15, 2024
@wakamex wakamex force-pushed the add_setuptools_pip_requirements branch from 8c4ad95 to 60d2829 Compare April 15, 2024 06:07
@wakamex wakamex reopened this Apr 15, 2024
@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

rebased. got it installed with eth_account==0.11.2 by uncapping eth-account in ape and eip712. also ape plugins list doesn't error on missing pkg_resources.

PS: lol pip can't install py-evm>=0.9. let's burn it with fire.

@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

now functional tests fail. I have no idea how difficult the required changes would be.

I think I went off the deep end trying not to use setuptools.

spinning off efforts:

  1. allow uv pip stand-in replacement if installed
  2. work toward reducing setuptools dependence (mostly in other packages that need to be upgraded) - spun off in feat: remove setuptools dependence #2003
  3. use uv to install ape in workflows - moved to WIP branch https://github.com/wakamex/ape/tree/use_uv_in_workflows
  4. update pinning strategy to stay more up-to-date with dependencies - think on it for now

Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Thanks

The pkg_resources problem is annoying. Thankfully it is going away anyway as part of adding Python 3.12 support.

src/ape/cli/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
src/ape/plugins/__init__.py Outdated Show resolved Hide resolved
src/ape/plugins/_utils.py Outdated Show resolved Hide resolved
@wakamex wakamex mentioned this pull request Apr 15, 2024
3 tasks
@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

having PIP_COMMAND in _utils creates a circular dependency:

  • from ._utils import PIP_COMMAND in src/ape/plugins/__init__.py because it's used in _plugin_modules
  • from ape.plugins import clean_plugin_name in src/ape/plugins/_utils.py reads in the __init__.py
  • rinse and repeat

this can be resolved by:

  1. moving the PIP_COMMAND definition to __init__.py (i think)
  2. moving code out of __init__.py to avoid importing from utils

fubuloubu
fubuloubu previously approved these changes Apr 15, 2024
src/ape/cli/__init__.py Outdated Show resolved Hide resolved
@wakamex
Copy link
Contributor Author

wakamex commented Apr 15, 2024

to resolve the circular dependency above, moved PluginManager to src/ape/managers/plugins.py and clean_plugin_name to ape/plugins/_utils

fubuloubu
fubuloubu previously approved these changes Apr 16, 2024
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Re-ran failed test case for Python 3.9

I'm okay with the move of the PluginManager but curious what @antazoey thinks

@antazoey
Copy link
Member

Re-ran failed test case for Python 3.9

I'm okay with the move of the PluginManager but curious what @antazoey thinks

I like the change but am worried if it's breaking.

@fubuloubu
Copy link
Member

fubuloubu commented Apr 17, 2024

Re-ran failed test case for Python 3.9
I'm okay with the move of the PluginManager but curious what @antazoey thinks

I like the change but am worried if it's breaking.

It lifts PluginManager from ape.plugins (which tbh should have been private e.g. ape._plugins) to the ape.managers namespace (which is also still not typically widely used except for typing maybe), lifting private stuff to public API should not be considered breaking

The expectation is that singletons from ape.managers end up in top level namespace, and also injected via ManagerAccessMixin

@wakamex
Copy link
Contributor Author

wakamex commented Apr 17, 2024

Looks like freak CI accident. Only MacOS Python 3.10 failed.

Here's the failing test. Looks like the CI runner just dropped a file or something.

     def test_handle_upgrade_result_when_upgrading_to_same_version(caplog, logger):
        plugin = PluginMetadata(name=THIRD_PARTY[0], version=f"0.{ape_version.minor}.0")
        handler = ModifyPluginResultHandler(plugin)
        handler.handle_upgrade_result(0, f"0.{ape_version.minor}.0")
        if records := caplog.records:
            assert (
                f"'{THIRD_PARTY[0]}' already has version '0.{ape_version.minor}.0'"
                in records[-1].message
            )
        else:
            version_at_end = plugin.version
>           pytest.fail(
                f"Missing logs when upgrading to same version 0.{ape_version.minor}.0. "
                f"version={version_at_end}"
            )
E           Failed: Missing logs when upgrading to same version 0.7.0. version=0.7.0

@fubuloubu
Copy link
Member

looks like github runners only support darwin-arm64 down to python 3.10.11 according to the CI error

   Error: The version '3.8' with architecture 'arm64' was not found for macOS 14.4.1.
  The list of all available versions can be found here: https://raw.githubusercontent.com/actions/python-versions/main/versions-manifest.json

Yes we are tracking to fix with #2020

@wakamex
Copy link
Contributor Author

wakamex commented Apr 22, 2024

awesome. let me know if this PR is missing anything. maybe we can revisit after #2020.

@antazoey
Copy link
Member

during testing I ran into an issue but fixed it here: #2021

antazoey
antazoey previously approved these changes Apr 23, 2024
@antazoey
Copy link
Member

antazoey commented Apr 23, 2024

Issue: uninstall doesn't work:

error: unexpected argument '-y' found

  tip: to pass '-y' as a value, use '-- -y'

Usage: uv pip uninstall [OPTIONS] <PACKAGE|--requirement <REQUIREMENT>>

For more information, try '--help'.
ERROR: Failed to uninstall plugin 'tokens.

edit: fixed w/ tests

@antazoey
Copy link
Member

antazoey commented Apr 23, 2024

issues: tests fail when uv is installed. it seems like we need to test that case more anyway.

edit: fixed

@antazoey antazoey force-pushed the add_setuptools_pip_requirements branch from 15c18a4 to 0e71415 Compare April 23, 2024 15:16
@antazoey antazoey changed the title feat: use uv if installed feat: use uv if installed Apr 23, 2024
@antazoey antazoey changed the title feat: use uv if installed feat: use uv if installed for ape plugins install / uninstall commands Apr 23, 2024
@antazoey
Copy link
Member

antazoey commented Apr 23, 2024

issue: pkg_resources shows up on trie 2.2.0, which is "installable" in current conditions (ran into this). Lucky, I think we can bump it to 3.0 and it will still work.

  • bump peer dependency trie to >=3.0.0,<4/

@antazoey antazoey force-pushed the add_setuptools_pip_requirements branch 2 times, most recently from 64b58be to 5b689b5 Compare April 23, 2024 15:58
@antazoey antazoey force-pushed the add_setuptools_pip_requirements branch from 5b689b5 to bd50c36 Compare April 23, 2024 15:59
@antazoey
Copy link
Member

Just realized - if this is working, then #1994 should too now.

@antazoey antazoey requested a review from mikeshultz April 23, 2024 18:04
@antazoey antazoey merged commit 01d6d73 into ApeWorX:main Apr 23, 2024
15 checks passed
@wakamex
Copy link
Contributor Author

wakamex commented May 13, 2024

just tested installation speeds between uv and pip, on latest 0.7.23 26bec8b:

tool uncached cached uncached unpinned pandas
uv 2:09.43 1.411 5.202
pip 2:29.40 8.867 9.192
speedup (s) 19.97 7.456 3.99
speedup (%) 13.37% 84.07% 43%

no fucking way
image

also, big speedup comes from loosening the pandas pin, which requires building numpy on my machine (Fedora 40)

@fubuloubu
Copy link
Member

Yeah, we actually want to remove the pandas pin eventually, it's not quite state of the art and end users might have a couple of different options they may want to choose from

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.

plugins not recognized when pip isn't installed
3 participants