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

Upgrade template #2128

Merged
merged 50 commits into from Jan 12, 2024
Merged

Upgrade template #2128

merged 50 commits into from Jan 12, 2024

Conversation

pacrob
Copy link
Collaborator

@pacrob pacrob commented Dec 18, 2023

What was wrong?

Pull in updates from the python project template, notably:

  • drop python 3.7 support
  • use pre-commit for linting
  • change the name of the master branch to main

Related to Issue #
Closes #1856 <- needs manual closing
Closes #1942

How was it fixed?

Todo:

  • Clean up commit history

  • Add or update documentation related to these changes

  • Add entry to the release notes

Cute Animal Picture

image

pacrob and others added 30 commits May 1, 2023 15:17
* convert bash scripts to py
* bump versions in dependencies and ci builds

* move tox to [dev] per issue ethereum#34

* move RTD deps pointer into .readthedocs.yml

* unpin flake8 add flake8-bugbear to lint deps
* remove gitter, testing setup, and pandoc sections, add quotes to dev install
* repin flake8, bump tox to >=4.0.0 as that's where whitelist was deprecated, misc updates
* template cleanup following initial merge with py-evm

* add flake8 pin comment

* correct license years

* add pin note to mypy
…m#81)

* apply template updates found following merge with eth-typing

* add build as a dev dependency

* remove timeout from pytest.ini, it doesn't do anything without pytest-timeout as a dep
* add updates found when merging template with py-ssz and eth-abi

* add wheel and wheel-windows to ci and reorg
* remove testall because it doesnt work
minor formatting updates, remove additional docs to separate pr
…sion

bump sphinx version and set py version rtd uses to 3.8
change references to doc to all be docs
Pre-release check for upstream configuration of remote
add `.venv*` to .gitignore
add `.build` to be ignored
fix indent, add sphinx fail_on_warning
gitignore local vs-code settings
* add pre-commit

* run pre-commit

* skip lint on README.md as it breaks template filling
@pacrob pacrob force-pushed the upgrade-template branch 10 times, most recently from 1d3ea54 to ae3ed26 Compare December 20, 2023 19:42
@pacrob pacrob marked this pull request as ready for review December 20, 2023 19:45
Copy link
Collaborator

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

This all looks in order, lgtm!

Copy link
Collaborator

@kclowes kclowes 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 taking this on! A few things I noticed:

  • It doesn't look like the core, database, difficulty, or vm tests are running
  • It looks like some upper pins were removed. I think I'd rather remove them all at the same time, so if that's too much for this PR, let's put those back in setup.py and make an issue to do that another time.
  • nit: it looks like precommit gets installed in the make lint section, so I don't think you need the separate build step for all the test runs
  • there are a few doc errors that could very well be user error :)


test-all:
tox
pytest tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be user error, but I'm not able to get pytest tests to run. We could either do pytest tests/core here, or pick a different command. I'm not sure what the best command to pick would be. I'm also not opposed to removing it altogether since I don't think it's super useful, but would want to check with @fselmo first since he's in here most often.

Copy link
Collaborator Author

@pacrob pacrob Dec 21, 2023

Choose a reason for hiding this comment

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

It needs the fixtures installed. CI does it with git submodule update --init --recursive, but the dev docs don't appear to mention it. Will add. It's in the contributing docs, git clone --recursive .... Installing Cloning that way allows make test run for me. You? Takes forever though 😄

Also, I'm seeing the tests you mention running in the CI "All checks have passed" list below. Where are you seeing that they didn't run?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that was likely my problem. I knew there was something funky to do with the submodule, but couldn't remember how to fix that.

If you look at the output in the run tox step here: https://app.circleci.com/pipelines/github/ethereum/py-evm/2555/workflows/fd9eaa7a-2157-4416-bda6-eaea85711132/jobs/172217
It's running in 17s, and I don't see the actual command for the tests (pytest tests/json-fixtures/test_difficulty.py). Looks like it usually takes ~2 minutes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests running correctly again!


coverage:
coverage run --source eth
coverage report -m
coverage html
open htmlcov/index.html

build-docs: clean
cd docs/; sphinx-build -W -T -E . _build/html
build-docs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may also be user error/not having the right things installed, but I'm seeing a bunch of warnings when I run make build-docs. For example:

  • WARNING: autodoc: failed to import module 'run' from module 'scripts.benchmark'; the following exception was raised: No module named 'checks'
  • ~/ef/py-evm/docs/tests.rst: WARNING: document isn't included in any toctree
    And:
Document: guides/building_an_app_that_uses_pyevm
------------------------------------------------
**********************************************************************
File "guides/building_an_app_that_uses_pyevm.rst", line 100, in default
Failed example:
    print(f"The balance of address {encode_hex(MOCK_ADDRESS)} is {mock_address_balance} wei"
Exception raised:
    Traceback (most recent call last):
      File "/opt/homebrew/Cellar/python@3.11/3.11.4_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/doctest.py", line 1351, in __run
        exec(compile(example.source, filename, "single",
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/Users/eve/ef/py-evm/venv-upgrade-template/lib/python3.11/site-packages/sphinx/ext/doctest.py", line 484, in compile
        return compile(code, name, self.type, flags, dont_inherit)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "<doctest default[10]>", line 1
        print(f"The balance of address {encode_hex(MOCK_ADDRESS)} is {mock_address_balance} wei"
             ^
    SyntaxError: '(' was never closed
**********************************************************************

Are you seeing those too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get the same errors, which makes me wonder why CI docs tests are passing. Looking into.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once tests got running again, this turned out to indeed be a ( I didn't close.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated
extras_require = {
"benchmark": [
"termcolor>=1.1.0,<2.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the upper bound here?

@@ -30,59 +49,21 @@
"test": [
"factory-boy==2.11.1",
"hypothesis>=5,<6",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to drop upper pins in this section too for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All upper pins gone except for towncrier and hypothesis.

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

I'd wait to see if @kclowes' comments are all addressed before merging but lgtm 👍🏼

@pacrob
Copy link
Collaborator Author

pacrob commented Jan 12, 2024

I think I've addressed everything you noted, but I'm not sure about:

  • nit: it looks like precommit gets installed in the make lint section, so I don't think you need the separate build step for all the test runs

Could you clarify?

@kclowes
Copy link
Collaborator

kclowes commented Jan 12, 2024

You're installing precommit on all test runs, but I think we just need it for the lint runs? And it looks like it may already get installed.
From the core tests, which shouldn't(?) need precommit:
image

From the lint tests - looks like a separate step isn't necessary? Not 100% sure on on that point though:
https://app.circleci.com/pipelines/github/ethereum/py-evm/2567/workflows/ea6769c2-b8f2-461b-9153-1aaea76ecb1c/jobs/172736?invite=true#step-109-57_61

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

:shipit: !

convert .format and % strings that pyupgrade can't do
drop unnecessary deps, remove unnecessary upper pins
remove pre-commit install from ci, it's handled by tox
@pacrob pacrob merged commit b5c9fd9 into ethereum:main Jan 12, 2024
45 checks passed
@pacrob pacrob deleted the upgrade-template branch January 12, 2024 22:22
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.

Remove linting error ignores
5 participants