Skip to content

Use non-editable installs for test generators#4634

Merged
jtraglia merged 1 commit into
ethereum:masterfrom
danceratopz:fix/vector-gen-with-process-based-pools
Oct 3, 2025
Merged

Use non-editable installs for test generators#4634
jtraglia merged 1 commit into
ethereum:masterfrom
danceratopz:fix/vector-gen-with-process-based-pools

Conversation

@danceratopz
Copy link
Copy Markdown
Member

@danceratopz danceratopz commented Oct 3, 2025

Fixes up a regression introduced in:

This issue describes the problem and fix:

This will cause the venv to flip-flop between installing eth2spec in editable and non-editable modes if you switch between non-generator/generator targets. But that should be ok; uv's good at that :)

image

@danceratopz danceratopz force-pushed the fix/vector-gen-with-process-based-pools branch from a92cb7c to 57e2149 Compare October 3, 2025 08:47
@danceratopz
Copy link
Copy Markdown
Member Author

Could a maintainer please trigger Generate Vectors to test the fix?

Repo:

danceratopz/consensus-specs

Ref:

fix/vector-gen-with-process-based-pools

@leolara leolara added the testing CI, actions, tests, testing infra label Oct 3, 2025
@etan-status
Copy link
Copy Markdown
Contributor

I'm not a maintainer but I ran it locally and it works fine now! Thanks for fixing it.

% rm -rf ../consensus-spec-tests/tests; \
    mkdir -p ../consensus-spec-tests/tests && \
    make clean && \
    make lint verbose=true && \
    caffeinate -ims make reftests && \
    find ../consensus-spec-tests/tests -depth -type d -empty -delete

Completed *::*::* in 2h 11m 35s

  Reference Test Generation Summary  
╭───────────┬──────────┬────────────╮
│ Metric    │    Count │ Percentage │
├───────────┼──────────┼────────────┤
│ Found     │   197562 │     100.0% │
│ Filtered  │        0 │       0.0% │
│ Selected  │   197562 │     100.0% │
│ Completed │    73619 │      37.3% │
│ Skipped   │   123943 │      62.7% │
│ Time      │ 7895.28s │            │
╰───────────┴──────────┴────────────╯

If we ever need pickling support, the C libraries can be wrapped easily, e.g., for lru-dict:

from collections.abc import Hashable
from typing import Any, Generic, TypeVar

from lru import LRU as OrigLRU

_KT = TypeVar("_KT", bound=Hashable)
_VT = TypeVar("_VT")


class LRU(Generic[_KT, _VT]):
    def __init__(self, size):
        self.cache_dict = OrigLRU(size=size)

    def __contains__(self, __o: Any) -> bool:
        return self.cache_dict.__contains__(__o)

    def __getitem__(self, item: _KT) -> _VT:
        return self.cache_dict.__getitem__(item)

    def __setitem__(self, key: _KT, value: _VT) -> None:
        self.cache_dict.__setitem__(key, value)

    def __reduce__(self):
        return (self.__class__, (self.cache_dict.get_size()))

with __reduce__ essentially describing the arguments with which __init__ should be called on the destination process. then, just import that module rather than the C library directly, caller code doesn't need other changes. BLS G1Point etc would likewise need a similar treatment.

However, #4633's long term plan suggests to move away from pickling entirely. Still leaving the comment in case it becomes an issue in the future.

@danceratopz
Copy link
Copy Markdown
Member Author

Thanks for verifying, @etan-status! And thanks for your alternative suggestion - your approach seems absolutely valid 👍

I'm happy if maintainers prefer to apply your suggestion over the less intrusive "hotfix" in this PR.

These are the reasons I took the approach in this PR:

  1. It maintains the status quo for vector generation from before Migrate config to pyproject.toml and use uv for project management #4627. I looked at similar solutions to yours, but wasn't sure if there could be over side effects/issues, see also next point :).
  2. I won't be able to PR a more involved approach, as I'll be afk for a wee while.
  3. As you mentioned, I lean towards this low effort hotfix as I believe we should put our efforts into unifying make test and make reftests to use the same test framework, i.e., to both use pytest. This would make your fix redundant, as it would work out the box in pytest via pytest-xdist (more info in Editable installs + multiprocessing break vector generation #4633).

@etan-status
Copy link
Copy Markdown
Contributor

I like this PR as is, my comment was more meant as further documentation in case pickling gets revisited later on. Status quo worked well, no reason to change it on a whim.

Copy link
Copy Markdown
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @danceratopz! Can confirm it fixes the issue.

image

@jtraglia jtraglia merged commit 5a4e05a into ethereum:master Oct 3, 2025
15 checks passed
jtraglia pushed a commit that referenced this pull request Oct 22, 2025
Adds `--reinstall-package=eth2spec` for `uv run` commands used with
editable installs. This will ensure that `uv` doesn't used a cached
version of `eth2spec` (consensus-specs) and ensure that the most recent
version of repo source is used when generating test vectors during local
development.

Follow-up fix to:
- #4627
 
and, in particular:
- #4634
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing CI, actions, tests, testing infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants