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

Add specific and actionable instructions to stale lockfile errors #12699

Merged

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Aug 30, 2021

This changes the lockfile staleness messages to include information on how to reconfigure pants to resolve the error. Language is largely similar to that proposed in #12654, with minor changes:

  • Language changes to explain the need for action on the user's part (you can fix this by rather than please change )

Closes #12654

Christopher Neugebauer added 6 commits August 30, 2021 13:26
# 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]
…r messages

# 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]
# 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]
[ci skip-rust]

[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]
@chrisjrn
Copy link
Contributor Author

OK, the tests don't pass due to timing out. Those will need a rewrite; one moment

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you Chris

src/python/pants/backend/python/goals/pytest_runner.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/typecheck/mypy/rules.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
else:
yield (
"To regenerate your lockfile based on your current configuration, run "
"`./pants generate-lockfiles`. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"`./pants generate-lockfiles`. "
"`./pants generate-lockfiles --resolve={tool_name}`. "

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump

src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
src/python/pants/backend/python/util_rules/pex_test.py Outdated Show resolved Hide resolved
Christopher Neugebauer added 4 commits August 30, 2021 16:53
# 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]
# 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]
# 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]
# 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]
@chrisjrn
Copy link
Contributor Author

I think I've caught everything here. Let me know if you've got any questions.

# 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]
elif isinstance(requirements, ToolCustomLockfile):
yield f"the lockfile at {requirements.file_path} "
else:
raise Exception("Check the type hinting on `requirements`")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type hints on requirements say that this line should never be hit, but I'm not sure mypy agrees.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better is raise AssertionError. And yeah this is good to keep. Although note that it will fail much earlier on line 619 due to AttributeError if it isn't a _ToolLockfileMixin.

Maybe you can assert that it's an instance of _ToolLockfileMixin? It is totally valid to not support invalidation for non-tool lockfiles in Pants 2.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it will definitely fail earlier.

I didn't really want to expose _ToolLockfileMixin, it's mostly there to ensure consistency of field names through DRY more than an implementation detail, we can change that if you think it helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to keep it private w/ the underscore, but still import it here. I strongly suspect this code will change in Pants 2.8 when we add support for multiple user lockfiles - so it makes sense for the type to be an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, my expectation is that we'll need a separate error message function to take care of user lockfiles

Comment on lines 136 to 137
extra_requirements: Iterable[str] = (),
uses_source_plugins: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify things by chaing extra_requirements to be requirements_from_source_plugins: Iterable[str] | None = None. If requirements_from_source_plugins is None, assume source plugins are not used.

Note that both MyPy and Pylint will pass an empty FrozenOrderedSet, rather than None, if no plugins are used:

@dataclass(frozen=True)
class MyPyFirstPartyPlugins:
requirement_strings: FrozenOrderedSet[str]

return PythonLockfileRequest.from_tool(
mypy, constraints, extra_requirements=first_party_plugins.requirement_strings
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Firstly, that line is stale, and I'll be deleting it shortly. I'm not sure that relying on this parameter being set to determine overall tool behaviour is a good idea, because the impact to 3rd-party plugin developers of whether they use None or a falsey iterator is pretty obscure at the moment. I've added a field at the class level, which is what we're actually using to implement the behaviour, which keeps things explicit.

Comment on lines 99 to 101
@dataclass(frozen=True)
class ToolProgrammaticLockfile(LockfileContent, _ToolLockfileMixin):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this makes sense to have without having call sites using this.

Instead, I think we simply don't support anything other than ToolCustomLockfile and ToolDefaultLockfile for now, as mentioned below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair

src/python/pants/backend/python/util_rules/pex.py Outdated Show resolved Hide resolved
elif isinstance(requirements, ToolCustomLockfile):
yield f"the lockfile at {requirements.file_path} "
else:
raise Exception("Check the type hinting on `requirements`")
Copy link
Contributor

Choose a reason for hiding this comment

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

Better is raise AssertionError. And yeah this is good to keep. Although note that it will fail much earlier on line 619 due to AttributeError if it isn't a _ToolLockfileMixin.

Maybe you can assert that it's an instance of _ToolLockfileMixin? It is totally valid to not support invalidation for non-tool lockfiles in Pants 2.7.

else:
yield (
"To regenerate your lockfile based on your current configuration, run "
"`./pants generate-lockfiles`. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Bump

Christopher Neugebauer added 2 commits August 31, 2021 11:58
# 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]
# 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]
elif isinstance(requirements, ToolCustomLockfile):
yield f"the lockfile at {requirements.file_path} "
else:
raise Exception("Check the type hinting on `requirements`")
assert isinstance(requirements, (ToolCustomLockfile, ToolDefaultLockfile))
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be moved higher up in the function. Once you do that, you can simply do if isinstance(requirements, ToolDefaultLockfile) then else (meaning ToolCustomLockfile). MyPy should have "tightened" the types for you appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I've added an assertion, mypy is complaining that it's unreachable. Much better!

deletes

Comment on lines 683 to 684
else:
message = "TODO: add support for user lockfiles"
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed. The funciton only supports tool lockfiles, meaning that you can't hook up the invalidation check for anything else in Pants 2.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced with a comment and a false assertion

Christopher Neugebauer added 2 commits August 31, 2021 12:31
# 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]
# 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]
# 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
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks Chris! Looks great

if (ir or ic)
],
)
def test_validate_metadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right - this does make a already big file even bigger and more complex. And this will only get worse when we add multiple user lockfiles.

What do you think about this home being in lockfile_metadata.py? Maybe it'd even make sense to rename to lockfile_invalidation.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good fix for the next round of changes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sgtm!

@Eric-Arellano Eric-Arellano merged commit 1cfe8dd into pantsbuild:main Aug 31, 2021
chrisjrn pushed a commit to chrisjrn/pants that referenced this pull request Aug 31, 2021
…ntsbuild#12699)

This changes the lockfile staleness messages to include information on how to reconfigure pants to resolve the error. Language is largely similar to that proposed in pantsbuild#12654, with minor changes:

* Language changes to explain the need for action on the user's part (_you can fix this by_ rather than _please change_ )

Closes pantsbuild#12654
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
stuhood pushed a commit that referenced this pull request Sep 2, 2021
…errypick of #12699). (#12717)

This changes the lockfile staleness messages to include information on how to reconfigure pants to resolve the error. Language is largely similar to that proposed in #12654, with minor changes:

* Language changes to explain the need for action on the user's part (_you can fix this by_ rather than _please change_ )

Closes #12654

Cherry-picks `1cfe8dd`.

[ci skip-rust]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lockfiles: teach our staleness error message about <default> tool lockfiles
2 participants