Markdown to SpecObject refactoring #4330

leolara · 2025-05-19T13:34:43Z

Summary

This pull request introduces a refactoring effort, focusing on modularizing and improving the maintainability of the specification parsing logic. The changes include updates to the Makefile, the addition of a new Python module (mk_to_spec.py). The primary goal is to replace the get_spec function with the new MarkdownToSpec class, ensuring functional equivalence during the transition phase.

Questions

I see that we categorise classes if they have @dataclass in one category, and if they don't have @dataclass and inherit from Container in another one. Error in any other case. For the second category wouldn't it make sense to belong there if you inherit from Container? Regardless if you have @dataclass or not?
In the current code, if a python block starts with a function or class but contains more stuff, all the source code gets indexed by the first element of the code block. With the new use of AST right now, it would be trivial to process each element independently, even if they are in the same code block. Should I do this? I think it gives more flexibility.
I detected that in at least one case we were adding a dataclass to the dataclass dictionary with None as index. I assumed this was a bug and I fixed it.
ConfigVars are typed as config_vars: Dict[str, VariableDefinition] but we are not using them that way, I guess I should change the typing to reflect how it is actually used?

Changes

1. New Module: `mk_to_spec.py`

Added a new Python module mk_to_spec.py under the pysetup directory.
This module encapsulates the logic for parsing markdown specification files and converting them into structured Python objects (SpecObject).
Full type hinting.
Use more extensive of AST library instead of string manipulation for parsing Python code.
Key features include:
- Parsing markdown tables, code blocks, and headings.
- Handling custom types, constants, presets, and configuration variables.
- Support for list-of-records tables and HTML comments for skip logic.
- Fixed a bug in the get_spec function where dataclass names were not being parsed correctly.

2. Makefile Updates

Modified the test target to include the tests/infra directory in the test suite.
Updated the clean target to only remove files that are generated by the build, and not remove all the files that are not being tracked.

3. Dependency Updates

Added deepdiff==8.5.0 to the pyproject.toml file for deep comparison of Python objects.

4. Enhancements to `setup.py`

Integrated the new MarkdownToSpec class from mk_to_spec.py.
Added a new function get_spec_new to utilize the MarkdownToSpec class for generating specification objects.
Updated the build_spec function to compare the new and old specification generation methods using DeepDiff to ensure consistency.

Refactoring Details

The MarkdownToSpec class is a refactored and enhanced version of the get_spec function, offering better modularity, maintainability, and feature support.
I have tried to make MarkdownToSpec identical in input/output to get_spec with the only difference that was previously adding classes with None index to the dataclass dictionary, and I think this was a bug.
The get_spec function remains in place temporarily to validate the functional equivalence of the new implementation.
The build_spec function includes assertions to compare the outputs of get_spec and MarkdownToSpec using DeepDiff.
Once the MarkdownToSpec class is reviewed and validated, the get_spec function will be removed, as well as auxiliary functions copied to pysetup/mk_to_spec.py and DeedDiff dependency.

Testing

Verified that the new MarkdownToSpec class produces consistent results with the existing get_spec function.
Added assertions in setup.py to ensure that the new and old methods produce identical outputs.
Updated the test suite to include the tests/infra directory. And added there test for the new MarkdownToSpec class.

Impact

Improves the maintainability of the codebase by modularizing the specification parsing logic.
Adds unit testing to this part of the code

…efactor

jtraglia

Hey @leolara, thanks for cleaning this up. I agree this is better than the current code. I've left some comments with various changes I'd like to see.

Makefile

pyproject.toml

pysetup/mk_to_spec.py

setup.py

jtraglia · 2025-05-20T13:15:01Z

pysetup/mk_to_spec.py

+    def run(self) -> SpecObject:
+        """
+        Orchestrates the parsing and processing of the markdown spec file.
+        - Calls _parse_document()
+        - Iterates over self.document.children and processes each child
+        - Calls _finalize_types() and _build_spec_object() after processing
+        Returns:
+            SpecObject: The constructed specification object.
+        """
+        while (child := self._get_next_element()) is not None:
+            self._process_child(child)
+        self._finalize_types()
+        return self._build_spec_object()


I'm not a fan of comments which just outline what the function does. Especially when it's wrong... For example, this function does not call _parse_document(). Also the list items are missing punctuation. Let's just simplify this (and remove the fancy "orchestrates" word, did AI write this function?). Maybe something like:

""" Returns parsed markdown specification. """

Sorry, it was the original implementation that was calling _parse_document, I will review the comments

pysetup/mk_to_spec.py

jtraglia · 2025-05-20T13:27:07Z

pysetup/mk_to_spec.py

+def _has_decorator(decorateable: ast.ClassDef | ast.FunctionDef, name: str) -> bool:
+    return any(_is_decorator(d, name) for d in decorateable.decorator_list)


Why is this function outside of the class? It's only used in the class.

jtraglia · 2025-05-20T13:28:35Z

tests/infra/test_mk_to_spec.py

Testing this is a good idea, thanks. Reminder to rename the file.

jtraglia · 2025-05-20T13:45:08Z

I see that we categorise classes if they have @dataclass in one category, and if they don't have @dataclass and inherit from Container in another one. Error in any other case. For the second category wouldn't it make sense to belong there if you inherit from Container? Regardless if you have @dataclass or not?

Hmm interesting question. I don't believe there should be a situation in which a class is both. Is there?

In the current code, if a python block starts with a function or class but contains more stuff, all the source code gets indexed by the first element of the code block. With the new use of AST right now, it would be trivial to process each element independently, even if they are in the same code block. Should I do this? I think it gives more flexibility.

Oh. Yes, it would be both fixing this. But in a follow up PR.

I detected that in at least one case we were adding a dataclass to the dataclass dictionary with None as index. I assumed this was a bug and I fixed it.

Was that the OptimisticStore dataclass? If so, I had been meaning to fix this. Thank you!

ConfigVars are typed as config_vars: Dict[str, VariableDefinition] but we are not using them that way, I guess I should change the typing to reflect how it is actually used?

I'm pretty sure this is due to our recent "list of records" support, right? Yeah, I'd either remove the type hint or correct it in some way; maybe Dict[str, Any] would be sufficient here. You can make that change here if you want.

jtraglia · 2025-05-20T15:35:30Z

Hey @leolara, I've merged the following PR. Could you pull in these changes & run make lint?

Run linting on pysetup directory #4335

jtraglia · 2025-05-20T15:37:20Z

Updated the clean target to remove additional directories and files, such as venv, .mypy_cache, and pysetup/__pycache__.

Also regarding this, the current version of make clean will remove these. For example:

$ make clean
Removing .mypy_cache/
Removing build/
Removing eth2spec.egg-info/
Removing tests/core/pyspec/eth2spec/altair/
Removing tests/core/pyspec/eth2spec/bellatrix/
Removing tests/core/pyspec/eth2spec/capella/
Removing tests/core/pyspec/eth2spec/deneb/
Removing tests/core/pyspec/eth2spec/eip6800/
Removing tests/core/pyspec/eth2spec/eip7441/
Removing tests/core/pyspec/eth2spec/eip7732/mainnet.py
Removing tests/core/pyspec/eth2spec/eip7732/minimal.py
Removing tests/core/pyspec/eth2spec/eip7805/
Removing tests/core/pyspec/eth2spec/electra/
Removing tests/core/pyspec/eth2spec/fulu/
Removing tests/core/pyspec/eth2spec/phase0/
Removing venv/

leolara · 2025-05-21T12:46:53Z

Updated the clean target to only remove files that are generated by the build, and not remove all the files that are not being tracked.

I updated the comment because it was explained wrong:

Updated the clean target to only remove files that are generated by the build, and not remove all the files that are not being tracked.

jtraglia · 2025-05-21T18:45:32Z

Updated the clean target to only remove files that are generated by the build, and not remove all the files that are not being tracked.

Ah but this is intentional. I want this to happen 🙂

leolara · 2025-05-22T05:13:02Z

leolara · 2025-05-22T07:20:50Z

I detected that in at least one case we were adding a dataclass to the dataclass dictionary with None as index. I assumed this was a bug and I fixed it.

Was that the OptimisticStore dataclass? If so, I had been meaning to fix this. Thank you!

Yes, that one, it is solved now because it uses the AST to get the name of the class

leolara · 2025-05-22T07:22:56Z

ConfigVars are typed as config_vars: Dict[str, VariableDefinition] but we are not using them that way, I guess I should change the typing to reflect how it is actually used?

I'm pretty sure this is due to our recent "list of records" support, right? Yeah, I'd either remove the type hint or correct it in some way; maybe Dict[str, Any] would be sufficient here. You can make that change here if you want.

I changed it to dict[str, str | list[dict[str, str]]] which correspond with the data. I think Any should be avoided if there is a posibility.

leolara · 2025-05-22T07:29:11Z

I see that we categorise classes if they have @dataclass in one category, and if they don't have @dataclass and inherit from Container in another one. Error in any other case. For the second category wouldn't it make sense to belong there if you inherit from Container? Regardless if you have @dataclass or not?

Hmm interesting question. I don't believe there should be a situation in which a class is both. Is there?

I understand there are two categories, what I am saying that perhaps the rule for which category could be simpler. Like Container is one category and not Container is another one, regardless of @dataclass. For example.

Currently the rule is:

Has @dataclass -> dataclass
Has not @dataclass and inherit Container -> szz_object
Any other class -> error

Should it be like this or is there a simplified version that doesn't have error? Like:

Inherit Container -> szz_class
otherwise -> class

jtraglia · 2025-05-22T12:44:56Z

I see that we categorise classes if they have @dataclass in one category, and if they don't have @dataclass and inherit from Container in another one. Error in any other case. For the second category wouldn't it make sense to belong there if you inherit from Container? Regardless if you have @dataclass or not?

Hmm interesting question. I don't believe there should be a situation in which a class is both. Is there?

I understand there are two categories, what I am saying that perhaps the rule for which category could be simpler. Like Container is one category and not Container is another one, regardless of @dataclass.

There's sort of an implicit check here. I don't think we want to allow classes that aren't one of those categories. I get what you're suggesting, I'm just not sure if it's a good idea. Might be best to save this for later, so we can think more about it.

Makefile

leolara · 2025-05-27T06:19:44Z

I think I have done everything you mentioned @jtraglia ; have another look and then I will remove the old implementation

jtraglia · 2025-05-27T14:00:05Z

I think I have done everything you mentioned @jtraglia ; have another look and then I will remove the old implementation

Yup, it looks great 😄 let's proceed.

leolara added 3 commits May 16, 2025 03:05

Initial checkpoint

e01a800

Second checkpoint

f0adad6

Third checkpoint

7afaf9f

github-project-automation bot added this to Lodestar Consensus Devnet Planning May 19, 2025

leolara added 3 commits May 20, 2025 11:44

Add tests

ac7a1dc

Merge remote-tracking branch 'upstream/dev' into leolara/mk_to_spec-r…

e77dfbe

…efactor

Remove whitespaces

42c9d46

leolara marked this pull request as ready for review May 20, 2025 12:30

jtraglia reviewed May 20, 2025

View reviewed changes

Fix a few more type hints

d411624

leolara added 2 commits May 21, 2025 16:50

Split in two lines

bdfeaaf

mk -> md

af4b2b4

leolara force-pushed the leolara/mk_to_spec-refactor branch from 5df98f3 to af4b2b4 Compare May 21, 2025 16:54

leolara and others added 3 commits May 21, 2025 23:55

Merge branch 'dev' into leolara/mk_to_spec-refactor

ad53bf6

Fix imports

56c55d3

Linting

23958cf

Remove blank line

a003052

Fix docstrings and some typing

7d66033

leolara closed this May 22, 2025

leolara reopened this May 22, 2025

leolara added 2 commits May 23, 2025 11:25

Restore makefile

9ea94a7

Add fruits

035a316

jtraglia reviewed May 23, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

leolara added 3 commits May 24, 2025 05:55

Fix parameter for git clean

3baab73

Linter

9f4ab4a

Reorder __init__.py

6595bdb

		def _has_decorator(decorateable: ast.ClassDef \| ast.FunctionDef, name: str) -> bool:
		return any(_is_decorator(d, name) for d in decorateable.decorator_list)

Markdown to SpecObject refactoring #4330

Are you sure you want to change the base?

Markdown to SpecObject refactoring #4330

Conversation

leolara commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Questions

Changes

1. New Module: mk_to_spec.py

2. Makefile Updates

3. Dependency Updates

4. Enhancements to setup.py

Refactoring Details

Testing

Impact

Uh oh!

jtraglia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtraglia May 20, 2025

Choose a reason for hiding this comment

Uh oh!

leolara May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtraglia May 20, 2025

Choose a reason for hiding this comment

Uh oh!

jtraglia May 20, 2025

Choose a reason for hiding this comment

Uh oh!

jtraglia commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtraglia commented May 20, 2025

Uh oh!

jtraglia commented May 20, 2025

Uh oh!

leolara commented May 21, 2025

Uh oh!

jtraglia commented May 21, 2025

Uh oh!

leolara commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leolara commented May 22, 2025

Uh oh!

leolara commented May 22, 2025

Uh oh!

leolara commented May 22, 2025

Uh oh!

jtraglia commented May 22, 2025

Uh oh!

Uh oh!

leolara commented May 27, 2025

Uh oh!

jtraglia commented May 27, 2025

Uh oh!

Uh oh!

leolara commented May 19, 2025 •

edited

Loading

1. New Module: `mk_to_spec.py`

4. Enhancements to `setup.py`

jtraglia commented May 20, 2025 •

edited

Loading

leolara commented May 22, 2025 •

edited

Loading