-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(trace): Spec Neotrace - automatic test tracing with smart state tracking #4755
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
base: master
Are you sure you want to change the base?
Conversation
|
|
tests/infra/trace/models.py
Outdated
|
|
||
| # Classes that should be treated as tracked SSZ objects in the trace. | ||
| # Maps class name -> context collection name. | ||
| CLASS_NAME_MAP: dict[str, str] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To know if an object should be considered like SSZ, we use this: https://github.com/ethereum/consensus-specs/blob/master/tests/infra/yield_generator.py#L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
View - got it, will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there are int subclasses and the like that are subclassing View and technically can be SSZ'd but probably should be stored directly in the trace for simplicity and compactness (Slot is one example I saw in tests). Any suggestion how to handle those?
tests/infra/trace/models.py
Outdated
|
|
||
| class ContextObjectsModel(BaseModel): | ||
| """ | ||
| Defines the SSZ objects (artifacts) loaded in the 'context' block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mapping should be by hash_root. All View have this method. No need to make the type part of the filename. No need to keep the mapping. Getting the hash_root will give you the filename
tests/infra/trace/models.py
Outdated
| """ | ||
|
|
||
| metadata: dict[str, Any] = Field(..., description="Test run metadata (fork, preset, etc.)") | ||
| context: ContextModel = Field(default_factory=ContextModel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is this for. It is not in the YAML example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover complexity (there was originally a way to customize artifact names so a mapping to keep track of them was necessary). Probably not required if we match everything by hash, will remove.
tests/infra/trace/models.py
Outdated
| _artifacts: dict[str, Container] = PrivateAttr(default_factory=dict) | ||
|
|
||
| def register_object(self, obj: Any) -> ContextVar | None: | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is need to register. The obj.hash_tree_root().hex() should be the name. Also, just store the filename. Perhaps, in the model we can store a type of object that contains the filename. Like SSZSerialised(filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense, having three separate ways to spell the hash is unnecessary. Will simplify.
tests/infra/trace/models.py
Outdated
|
|
||
| return context_name | ||
|
|
||
| def dump_to_dir(self, output_dir: str, config: dict[str, Any] = None) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the normal pydantic way of saving to have this as a method of the model, or having externally in another function? We should do it the pydantic way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check... I'm more used to seeing it as a method but let's do idiomatic 👍
tests/infra/trace/models.py
Outdated
| print(f"ERROR: Failed to write YAML {path}: {e}") | ||
|
|
||
|
|
||
| class ConfigModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used. Why is it here?
tests/infra/trace/models.py
Outdated
| config: dict[str, Any] = Field(..., description="Dictionary of config constants") | ||
|
|
||
|
|
||
| class MetaModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used. Why is it here?
tests/infra/trace/decorator.py
Outdated
|
|
||
| def decorator(fn: Callable): | ||
| @functools.wraps(fn) | ||
| def wrapper(*args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too long, are you sure we need to do all this? I think we just need to wrap spec. If we need it move some things to other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's simplify further. On it 👷♂️ Thank you for feedback, this helps a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what is happening, you are trying to work out details about the tests to decide the name of where to store it.
The thing is that the decorator is not the best place for this. It is the test runner, so it is better if the decorator just returns the trace as an object and in the runner we do the saving to file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ookay, I think I understand... Should the result be returned in format compatible with default Dumper (generator of triplet tuples) or should this come with a custom runner/dumper that supports unwrapping pydantic objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not compatible with the dumper. I think we need to return the pydantic model. Then make sure that parts that expect the yields up the calling stack also can let this type pass through. Then in this function: https://github.com/ethereum/consensus-specs/blob/master/tests/core/pyspec/eth2spec/gen_helpers/gen_base/gen_runner.py#L87 we handle differently if it is returned a iterator/generator or the pydantic mode. Then there if there is a pydantic model we dump it. In this function test_case contains all the meta info about the test we are running, so that way you can calculate the folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
So far I have made it functional with the decorator just yielding things (it's in a separate branch for now, works but looks a little weird), it should be trivial enough to modify that function to detect return type instead and make the decorator just return the instance.
I also addressed all the other points (or almost, still rechecking) and aligned format details, etc. with the description in the original issue as well as I could. Should be ready for another review soon.
tests/infra/trace/models.py
Outdated
| params: dict[str, Any] = Field( | ||
| default_factory=dict, description="Arguments passed to the function" | ||
| ) | ||
| result: Any | None = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of this fields doesn't match the issue, like this, it is assert_output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, method is missing. I think we need a structure with an abstract base class. Where op defines the subclass
tests/infra/trace/models.py
Outdated
| Represents a function call ('op'), its inputs, and its outcome. | ||
| """ | ||
|
|
||
| op: str = Field(..., description="The spec function name, e.g., 'process_slots'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, op is not this
pydantic models for the spec trace core spec tracing logic use wrapt to wrap the spec and intercept the calls tracing decorator some basic unit tests for the trace recorder some converted test examples use 0x prefix for hex bytes in trace a README with a short explanation how tracing works
add "method" to StepModel for spec_call op remove unneeded things address some more requirements, format, etc. new approach - decorator just generates data for dumper add the auto-assert of state in the end of test trace adjust assert/load tracing logic according to the issue rename record_spec_trace -> spec_trace test fixes more simplicity some cleanup
f0dfc04 to
ec8d235
Compare
this still uses generator functions but adds a new data type functional but probably could be implemented better
|
https://discord.gg/the-arenaUna disculpa espero no se mal intérprete como una grosería de mi parte, pero no comprendo ni siquiera un 40% otro idioma, y menos temas tan específicos como programación y sistemas, sinceramente les admiro su labor como programadores, es algo que yo reconozco no se hacer, pero suelo tener en ocasiones buenas ideas, y me gusta aferrarme por el bienestar del equipo a que se hagan de la mejor manera posible, si les interesa pueden checar - [ ] este grupo |
decorator is still applied lazily but it's using wrapt factory now models are little cleaner and produce more uniform traces some data sanitization logic was streamlined
|
@leolara if you could just take another glance? :) I'm not sure about the best way to integrate object-returning tests into the same harness as generator tests. I did something and it works with reftests and has minimal impact on existing code but it doesn't look quite right, yet I don't want to rewrite half the test framework for supporting this either, at least not without some guidance. |
This PR provides a major part of a new testing framework - spec wrapper with automatic test execution tracing and smart beacon state tracking. It's based on #4603 and lessons learned in #4724 were taken into account.
This is fully backwards-compatible thing, new test/infra module was added, no preexisting code changed.
Key Changes:
Smart State Tracking: The RecordingSpec now automatically detects state context switches by tracking the root hash of the state argument. It prepends a load_state operation only when the state has actually changed (including out-of-band mutations in tests), removing the need for manual yields and the like.
Pydantic Serialization: Leverages Pydantic field_validator and model_dump to automatically handle type coercion (e.g., converting bytes to hex strings, int subclasses to primitives) and sanitation.
Lean Proxy: Reduced RecordingSpec (based on wrapt) to a thin wrapper that strictly handles function interception and flow control.
Testing: Added unit tests for the new tools and example spec tests based on the new infra. All tests are passing, lint was done.
Fixes #4603
Related to #4724