[CHIA-598] virtual project structure

[arvidn]

Purpose:

Slightly modified subset of @Quexington 's virtual project structure tool. #17810

The main differences are:

• All files default to be treated as belonging to the chia-blockchain project.
• There are no annotations (yet)
• There are no exclusions (but it's still supported)

This is immediately helpful for someone breaking out a sub-project to prevent other (unrelated) PRs re-introducing dependency cycles.

Current Behavior:

There is no way to "lock in" progress of breaking out sub-projects that should not have dependencies back into the core of chia-blockchain.

New Behavior:

This tool allows you to "lock in" progress of breaking out sub-projects. Once it lands, other PRs cannot re-introduce dependency cycles without CI turning red.

Changes:

Changes from Quexingtons patch (it's not so easy to compare on github because we have different base commits).

diff --git a/chia/util/virtual_project_analysis.py b/chia/util/virtual_project_analysis.py
index 36513f2c03..2b6c31c13e 100644
--- a/chia/util/virtual_project_analysis.py
+++ b/chia/util/virtual_project_analysis.py
@@ -1,5 +1,3 @@
-# Package: development
-
 from __future__ import annotations

 import ast
@@ -14,34 +12,44 @@ from typing import Any, Callable, Dict, List, Literal, Optional, Tuple, Union
 import click
 import yaml

+# This tool enforces digraph dependencies within a "virtual project structure".
+# i.e. files grouped together forming a project are not allowed to have cyclical
+# dependencies on other such groups.
+
+# by default, all files are considered part of the "chia-blockchain" project.
+
+# To pull out a sub project, annotate its files with a comment (on the first
+# line):
+# Package: <name>
+
+# if chia-blockchain depends on this new sub-project, the sub-project may not
+# depend back on chia-blockchain.
+

 @dataclass(frozen=True)
 class Annotation:
     package: str
-
-    @classmethod
-    def is_annotated(cls, file_string: str) -> bool:
-        return file_string.startswith("# Package: ")
+    is_annotated: bool

     @classmethod
     def parse(cls, file_string: str) -> Annotation:
         result = re.search(r"^# Package: (.+)$", file_string, re.MULTILINE)
         if result is None:
-            raise ValueError("Annotation not found")
+            return cls("chia-blockchain", False)

-        return cls(result.group(1).strip())
+        return cls(result.group(1).strip(), True)


 @dataclass(frozen=True)
 class ChiaFile:
     path: Path
-    annotations: Optional[Annotation] = None
+    annotations: Annotation

     @classmethod
     def parse(cls, file_path: Path) -> ChiaFile:
         with open(file_path, encoding="utf-8", errors="ignore") as f:
             file_string = f.read().strip()
-            return cls(file_path, Annotation.parse(file_string) if Annotation.is_annotated(file_string) else None)
+            return cls(file_path, Annotation.parse(file_string))


 def build_dependency_graph(dir_params: DirectoryParameters) -> Dict[Path, List[Path]]:
@@ -321,19 +329,19 @@ def config(func: Callable[..., None]) -> Callable[..., None]:
     )
     def inner(config_path: Optional[str], *args: Any, **kwargs: Any) -> None:
         exclude_paths = []
-        ignore_cycles_in = []
-        ignore_specific_files = []
-        ignore_specific_edges = []
+        ignore_cycles_in: List[str] = []
+        ignore_specific_files: List[str] = []
+        ignore_specific_edges: List[str] = []
         if config_path is not None:
             # Reading from the YAML configuration file
             with open(config_path) as file:
                 config_data = yaml.safe_load(file)

             # Extracting required configuration values
-            exclude_paths = [Path(p) for p in config_data.get("exclude_paths", [])]
-            ignore_cycles_in = config_data["ignore"].get("packages", [])
-            ignore_specific_files = config_data["ignore"].get("files", [])
-            ignore_specific_edges = config_data["ignore"].get("edges", [])
+            exclude_paths = [Path(p) for p in config_data.get("exclude_paths") or []]
+            ignore_cycles_in = config_data["ignore"].get("packages") or []
+            ignore_specific_files = config_data["ignore"].get("files") or []
+            ignore_specific_edges = config_data["ignore"].get("edges") or []

         # Instantiate DirectoryParameters with the provided options
         dir_params = DirectoryParameters(
@@ -366,7 +374,7 @@ def config(func: Callable[..., None]) -> Callable[..., None]:
 def find_missing_annotations(config: Config) -> None:
     flag = False
     for file in config.directory_parameters.gather_non_empty_python_files():
-        if file.annotations is None:
+        if not file.annotations.is_annotated:
             print(file.path)
             flag = True

Whatever dict-like object yaml.safe_load() returns, it doesn't appear its get() function accepts a default value. Whith 0 exclusions, it returned None instead of [].

[Quexington]

I'd prefer a version still with annotations (that all say chia-blockchain) so that we are forced to think about it. I also don't really see the point of this step except to add pre-commit & CI overhead when the path forward is uncertain. Those concerns being stated, I won't block this if others care to see it merged.

[richardkiss]

This file does not belong here. I'd prefer to see it in a whole other repo because I believe it it a generally useful tool that is more likely to evolve if it's not tied deeply to this chia-blockchain repo. I realize that's a bit tricky because of the git pre-commit action. But at the very least, can we move it to tools where quex had it?

[richardkiss]

My stand-alone version here https://github.com/richardkiss/vpa changes ChiaFile to PythonFile (for example). It also fixes some a problem with finding imports of the form from .xxx import yyy that this version misses, which could give false hope in places, and is part of the way to finding "local" imports hidden in function calls.

[arvidn]

I'd prefer to see it in a whole other repo because I believe it it a generally useful tool that is more likely to evolve if it's not tied deeply to this chia-blockchain repo

I agree with that sentiment, it's not very practical right now though, as it would add quite a lot of friction to iterate on the tool.

But at the very least, can we move it to tools where quex had it?

I'm pretty sure I didn't move any files from quex's PR. But tools seems reasonable to me.

[arvidn]

I moved it into tools/

[arvidn]

@richardkiss this worked locally for some reason, but I believe the issue of arbitrary directories in the root not being proper modules was the reason we moved tests into chia.

Did I misunderstand what you meant or is it OK to drop the last commit?

https://github.com/Chia-Network/chia-blockchain/actions/runs/10989286576/job/30507229355?pr=18616#step:14:7649

[altendky]

can we please just ban new Python code outside of chia/? i'm tired of maintaining the tool configs for all our Python code we've spread to the wind. if we do put the code outside then i guess we should recreate a tests directory outside. having a test in the package for code that is not in the package seems a touch silly. then we can update ci to account for multiple different tests directories. also, then we can have many more programs to run without being able to just to chia dev -h and see all our tooling listed with brief descriptions. /s (except i'm serious about just banning new Python files outside of chia/)

@richardkiss this worked locally for some reason, but I believe the issue of arbitrary directories in the root not being proper modules was the reason we moved tests into chia.

other projects use our test tooling. that's why the tests directory moved into the chia directory, so it would be available to the other projects. also i like to have tests available with the package rather than requiring a vcs checkout for that, but this is a side note. also also, i like fewer directories to have to configure differently in different tools...

[arvidn]

I just dropped the last commit, to restore the PR to its original form

[richardkiss]

Kyle doesn't think it should be outside chia. I don't think it should be in chia (or in this repo at all). But it's pretty hard (although not impossible) to incorporate it into github actions if it's outside this repo and Arvid thinks doing so would be impractical and increase friction. So we don't really have a consensus here.

What I think we should do: keep it external. Do a pip install git+https:// pinned by revision number (at least for now) in the github action. This allows extremely fine-grained use of the tool from an external repo without even doing a pypi release of it.

I am happy to take more ownership of this tool, externally, including some known bugs. But I don't want to get into the github action stuff because it seems like there are a lot of implicit opinions in how it works that I don't want to get involved in. And I don't know how this affects the check-in action. Although I think if there is a CI step that blocks PRs, that's probably enough.

[richardkiss]

@richardkiss this worked locally for some reason, but I believe the issue of arbitrary directories in the root not being proper modules was the reason we moved tests into chia.

Did I misunderstand what you meant or is it OK to drop the last commit?

https://github.com/Chia-Network/chia-blockchain/actions/runs/10989286576/job/30507229355?pr=18616#step:14:7649

FYI: you can go python tools/virtual_project_analysis.py to run things when there's no __init__.py. Although I'm not sure that's what we should do here.

[arvidn]

I also don't really see the point of this step except to add pre-commit & CI overhead when the path forward is uncertain.

It's this: "The main point of the tool is of course to prevent people not working on refactoring from undo-ing the refactor-ers' progress"

[Quexington]

It's this: "The main point of the tool is of course to prevent people not working on refactoring from undo-ing the refactor-ers' progress"

Yes, except the method with which to refactor has not been established and therefore progress cannot be made to potentially undo.

[github-actions]

File	Coverage	Missing Lines
chia/util/virtual_project_analysis.py	98.2%	lines 97, 154, 198, 204, 532

Total	Missing	Coverage
498 lines	5 lines	98%

[coveralls-official]

Pull Request Test Coverage Report for Build 10994621633

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 493 of 498 (99.0%) changed or added relevant lines in 2 files are covered.
• 29 unchanged lines in 8 files lost coverage.
• Overall coverage increased (+0.04%) to 90.983%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
chia/util/virtual_project_analysis.py	268	273	98.17%

Files with Coverage Reduction	New Missed Lines	%
chia/full_node/full_node_api.py	1	82.28%
chia/wallet/wallet_node.py	2	88.51%
chia/timelord/timelord_launcher.py	2	70.55%
chia/_tests/core/test_farmer_harvester_rpc.py	2	98.02%
chia/rpc/rpc_server.py	3	87.83%
chia/server/node_discovery.py	3	79.26%
chia/full_node/full_node.py	5	86.6%
chia/timelord/timelord.py	11	79.31%

Totals
Change from base Build 10966863149:	0.04%
Covered Lines:	102442
Relevant Lines:	112569

---

💛 - Coveralls

[arvidn]

Yes, except the method with which to refactor has not been established and therefore progress cannot be made to potentially undo.

you break something off by marking it as belonging to a different project. Then it can not be undone by mistake, only by changing back (or removing) the marking, or by disabling the CI job

[arvidn]

What I think we should do: keep it external. Do a pip install git+https:// pinned by revision number (at least for now) in the github action. This allows extremely fine-grained use of the tool from an external repo without even doing a pypi release of it.

You would still need two steps to make a change. e.g. I might want to add a feature where you can label files based on a glob pattern in the yaml file. I would have to add that feature in a separate repo and (possibly) not be able to demonstrate that it works as intended until I update it in chia-blockchain.

[richardkiss]

What I think we should do: keep it external. Do a pip install git+https:// pinned by revision number (at least for now) in the github action. This allows extremely fine-grained use of the tool from an external repo without even doing a pypi release of it.

You would still need two steps to make a change. e.g. I might want to add a feature where you can label files based on a glob pattern in the yaml file. I would have to add that feature in a separate repo and (possibly) not be able to demonstrate that it works as intended until I update it in chia-blockchain.

When you add a feature to vpa, you can also add a test. Or try the feature by hand. Or push it to a temporary branch and push a chia-blockchain PR with it pinned to that change #. Yes, an extra push, so slightly less convenient.