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

DM-35312 Support typing of ConfigurableAction(fields) #692

Merged
merged 1 commit into from Jun 28, 2022

Conversation

natelust
Copy link
Contributor

Support supplying Field types through python class typing syntax.
Another minor fix includes setting an identity attribute on an
action if it is accessed via a Field.

Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

Edit: this overview blurb is mostly about the pex_config PR; I just put it here by accident, but now that it's out in the world, it's not worth moving.

Lots of minor comments (maybe about needing more __get__ overloading and bad indentation - am I confused about whether black is being run on pex_config or something?).

My main concern from a big-picture standpoint is that there's no py.typed file, no MYPYPATH-setting in the EUPS table file, and more importantly no mypy.ini, github action or even any expectation that running mypy here would yield anything other than a ton of errors. So I think it's just people who have pylance configured a certain way who might benefit from the static type checking this enables, and I bet the cardinality of that set of people is of order unity.

So, that's not a blocker for merging, and I don't think expanding this ticket to include even really permissive mypy checking is a good idea either. And you've got tests that the runtime side of this works, and that helps a lot. But the static typing here is going to be super fragile as long as your editor is the only thing using it, and you should be mentally prepared for any typing that depends on it to be frequently broken (well, about as frequently as pex_config is modified at all, which of course isn't that frequent).

if attr == 'identity':
return object.__setattr__(self, attr, value)
return super().__setattr__(attr, value, at, label)

def __call__(self, *args, **kwargs) -> Any:
Copy link
Member

@TallJimbo TallJimbo Jun 24, 2022

Choose a reason for hiding this comment

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

This is preexisting, and it seems like something we might have even discussed already, but doesn't this signature imply that all ConfigurableAction subclasses must accept arbitrary *args, **kwargs, rather than the concrete signature declared by some intermediate ConfigurableAction abstract subclass? (This is exactly the same problem as Task.run, but without the history that makes that one harder to remove, I think).

In any case, it seems to me to make more sense to drop the __call__ here and define it only in subclasses like AnalysisAction that have a concrete signature. If the goal here is to be able to define some concrete ConfigurableAction subclasses here that have generic __call__ signatures because they're just forwarders, I think there's a better way to type that with PEP-612 stuff, though I don't know more than that.

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 will look more into PEP-612 stuff before merging this but AFAICT type checkers have no problem with this from a substitution principal, and I am not sure why. I dont know if they treat __ methods as special, or if __call__ is special because it is used in construction in so many parts of the language.

Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

Accidentally put my big-picture comments on the pipe_task PR; please see that for the overview.

Support supplying Field types through python class typing syntax.
Another minor fix includes setting an identity attribute on an
action if it is accessed via a Field.
@natelust natelust merged commit 6d65848 into main Jun 28, 2022
@natelust natelust deleted the tickets/DM-35312 branch June 28, 2022 16:18
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.

None yet

2 participants