Skip to content

Conversation

Carsons-Eels
Copy link
Contributor

What was wrong?

In order to WELD we're going to need to align on tooling, and this is an attempt to see how painful it will be to migrate to uv and ruff, leaving behind flake8, black, and isort (the latter two are covered by rules in ruff).

Related to Issue #

How was it fixed?

Updated the project config

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@Carsons-Eels Carsons-Eels added this to the Weld EEST+EELS milestone Jul 16, 2025
@Carsons-Eels Carsons-Eels added A-tool Area: tooling C-enhance Category: a request for an improvement E-medium Experience: of moderate difficulty P-medium labels Jul 16, 2025
@Carsons-Eels Carsons-Eels changed the title WELD tooling (switch over to uv+ruff) chore: WELD tooling (switch over to uv+ruff) Jul 21, 2025
@SamWilsn SamWilsn force-pushed the forks/osaka branch 3 times, most recently from 301eb89 to 5a49b2f Compare July 25, 2025 19:55
@Carsons-Eels Carsons-Eels force-pushed the WELD_tooling branch 4 times, most recently from c1e9043 to d07a600 Compare July 31, 2025 17:47
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 77.10843% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.25%. Comparing base (fab59d2) to head (2980f6b).
⚠️ Report is 2 commits behind head on forks/osaka.

Files with missing lines Patch % Lines
src/ethereum/osaka/vm/instructions/block.py 0.00% 3 Missing ⚠️
src/ethereum/berlin/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/byzantium/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/cancun/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/constantinople/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/frontier/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/homestead/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/istanbul/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/london/vm/gas.py 0.00% 2 Missing ⚠️
src/ethereum/osaka/bloom.py 0.00% 2 Missing ⚠️
... and 10 more
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@               Coverage Diff               @@
##           forks/osaka    #1326      +/-   ##
===============================================
- Coverage        94.26%   94.25%   -0.01%     
===============================================
  Files              583      583              
  Lines            34666    34665       -1     
  Branches          3070     3070              
===============================================
- Hits             32677    32675       -2     
- Misses            1454     1455       +1     
  Partials           535      535              
Flag Coverage Δ
unittests 94.25% <77.10%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Carsons-Eels Carsons-Eels self-assigned this Aug 1, 2025
@Carsons-Eels Carsons-Eels requested a review from SamWilsn August 5, 2025 12:10
@Carsons-Eels Carsons-Eels marked this pull request as ready for review August 5, 2025 12:10
@Carsons-Eels Carsons-Eels requested a review from gurukamath August 5, 2025 12:11
@Carsons-Eels Carsons-Eels force-pushed the WELD_tooling branch 3 times, most recently from 3b90303 to d43f696 Compare August 20, 2025 05:52
@Carsons-Eels Carsons-Eels force-pushed the WELD_tooling branch 2 times, most recently from 8405348 to 69c991e Compare August 20, 2025 15:35
SamWilsn
SamWilsn previously approved these changes Aug 20, 2025
@SamWilsn SamWilsn self-requested a review August 20, 2025 19:56
@SamWilsn SamWilsn dismissed their stale review August 20, 2025 19:57

premature.

@SamWilsn
Copy link
Contributor

@Carsons-Eels Carsons-Eels force-pushed the WELD_tooling branch 2 times, most recently from b68880e to 4b16b89 Compare August 22, 2025 00:29
@@ -212,6 +216,7 @@ def __call__(
[evm]: ref:ethereum.frontier.vm.Evm
[`TraceEvent`]: ref:ethereum.trace.TraceEvent
"""
del evm, event
Copy link
Contributor

Choose a reason for hiding this comment

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

Does raise NotImplementedError also mute the warning? I think raising that error would be more semantically correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked, yep it does. I'll switch it over

tox.ini Outdated
-m "not slow and not zkevm and not benchmark" \
-n auto --maxprocesses 6 \
-n auto --maxprocesses 10 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought we had updated this to 10, but then it switched back lower to 6. Was that because of impacts to the performance of the CI machine? If so, then I'll revert it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah exactly. We were OOMing

Copy link
Contributor

Choose a reason for hiding this comment

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

These are flipped I think? json_infra used to have 10 while py3 had 6.

@Carsons-Eels Carsons-Eels force-pushed the WELD_tooling branch 8 times, most recently from 2c30dd7 to 981d13f Compare August 26, 2025 19:33
) -> None:
"""
An [`EvmTracer`] that discards all events.
[`EvmTracer`]: ref:ethereum.trace.EvmTracer
"""

raise NotImplementedError
Copy link
Contributor

Choose a reason for hiding this comment

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

This one needs to be del because it actually gets called. If I commented on this one, my bad 😅

tox.ini Outdated
-m "not slow and not zkevm and not benchmark" \
-n auto --maxprocesses 6 \
-n auto --maxprocesses 10 \
Copy link
Contributor

Choose a reason for hiding this comment

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

These are flipped I think? json_infra used to have 10 while py3 had 6.

@SamWilsn SamWilsn merged commit 6960c5e into ethereum:forks/osaka Aug 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tool Area: tooling C-enhance Category: a request for an improvement E-medium Experience: of moderate difficulty P-medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants