Skip to content
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

Adding DigestTrie/Snapshot diffing #14872

Merged
merged 8 commits into from
Mar 23, 2022
Merged

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented Mar 22, 2022

In order to handle the upcoming change to "unify" both fmt and lint processes for formatters, we need to have the ability to find what files changed in a formatter rule run. This PR adds a recursive diff function to the Rust DigestTrie, which is also exposed via the PySnapshot/Snapshot Python byding/type.

The diff method returns a complete set of differences (ours/theirs files/dirs + changed) as this was readily available and could be useful in other contexts. For the purpose of my future changes, I just need the changed files.

Tests have been added Python-side as that is easiest to author/grok.

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon thejcannon added the category:internal CI, fixes for not-yet-released features, etc. label Mar 22, 2022
@thejcannon thejcannon requested a review from stuhood March 22, 2022 14:20
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
src/python/pants/engine/fs.py Show resolved Hide resolved
src/python/pants/engine/fs_test.py Show resolved Hide resolved
src/rust/engine/fs/src/directory.rs Show resolved Hide resolved
Comment on lines 85 to 86
# Use pants.engine.fs.SnapshotDiff for semantic names
def diff(
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we just prefix this with an underscore to really ward off callers?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that might make sense. In practice, users will probably consume this via a @rule that computes a diff string in some format using difflib...? And so this would be an implementation detail of that @rule rather than something consumed directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plan is for this to be called in FmtResult.message to compute the message. Well not "this" but pants.engine.fs.SnapshotDiff.from_snapshots.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@thejcannon
Copy link
Member Author

The test that fails seems unrelated to my changes (the new tests pass in the output)

=================================== FAILURES ===================================
_________________________ test_workspace_in_goal_rule __________________________

    def test_workspace_in_goal_rule() -> None:
        class WorkspaceGoalSubsystem(GoalSubsystem):
            name = "workspace-goal"
    
        class WorkspaceGoal(Goal):
            subsystem_cls = WorkspaceGoalSubsystem
    
        @dataclass(frozen=True)
        class DigestRequest:
            create_digest: CreateDigest
    
>       @rule
        def digest_request_singleton() -> DigestRequest:

src/python/pants/engine/fs_test.py:996: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/python/pants/engine/rules.py:338: in rule
    return inner_rule(*args, **kwargs, rule_type=RuleType.rule, cacheable=True)
src/python/pants/engine/rules.py:328: in inner_rule
    return rule_decorator(*args, **kwargs)
src/python/pants/engine/rules.py:249: in rule_decorator
    type_hints = get_type_hints(func)
/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/typing.py:1013: in get_type_hints
    value = _eval_type(value, globalns, localns)
/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/typing.py:263: in _eval_type
    return t._evaluate(globalns, localns)
/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/typing.py:467: in _evaluate
    eval(self.__forward_code__, globalns, localns),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   NameError: name 'DigestRequest' is not defined

<string>:1: NameError
- generated xml file: /tmp/process-executionbfcqNX/src.python.pants.engine.fs_test.py.tests.xml -


=========================== short test summary info ============================
FAILED src/python/pants/engine/fs_test.py::test_workspace_in_goal_rule - Name...
========================= 1 failed, 57 passed in 7.51s =========================

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

src/python/pants/engine/fs.py Show resolved Hide resolved
Comment on lines 85 to 86
# Use pants.engine.fs.SnapshotDiff for semantic names
def diff(
Copy link
Member

Choose a reason for hiding this comment

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

Yea, that might make sense. In practice, users will probably consume this via a @rule that computes a diff string in some format using difflib...? And so this would be an implementation detail of that @rule rather than something consumed directly.

src/rust/engine/fs/src/directory.rs Show resolved Hide resolved
src/rust/engine/fs/src/directory.rs Show resolved Hide resolved
src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved
@stuhood
Copy link
Member

stuhood commented Mar 22, 2022

The test that fails seems unrelated to my changes (the new tests pass in the output)

Related to the addition of from __future__ import annotations, perhaps...?

@thejcannon
Copy link
Member Author

The test that fails seems unrelated to my changes (the new tests pass in the output)

Related to the addition of from __future__ import annotations, perhaps...?

Ahh! I think the rule parsing code should be flexible enough to handle this, but we can workaround it here.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@thejcannon thejcannon merged commit 681944c into pantsbuild:main Mar 23, 2022
@thejcannon thejcannon deleted the triediff branch March 23, 2022 16:54
@benjyw benjyw mentioned this pull request Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants