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

Base classes for configuring and resolving python tools. #6870

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Dec 5, 2018

This was factored out and generalized from the existing isort
subsystem and resolution task.

Base classes for configuring and resolving python tools.
This was factored out and generalized from the existing isort
subsystem and resolution task.

@benjyw benjyw requested a review from baroquebobcat Dec 5, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good. One question about the factoring.

# product produced by this task. It is distinct from the subsystem type so that multiple
# instances of the same tool, possibly at different versions, can be resolved by different
# prep tasks, if necessary.
tool_instance_cls = None

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

Rather than specifying it this way, would it be reasonable to give the tool_subsystem_cls an abstract method that constructs the tool instance? Or could the abstract method just run the tool, and skip having the extract interface?

This comment has been minimized.

@benjyw

benjyw Dec 5, 2018

Contributor

Then what would the type of the product be? It shouldn't be the subsystem type because you can imagine multiple instances of the same subsystem class, in different scopes.

This comment has been minimized.

@benjyw

benjyw Dec 5, 2018

Contributor

We really only need this type to serve as the product type for the purpose of declaring product deps. Other than that, yes, we could construct the actual product itself from a generic "PythonTool" class.

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

Ah, I see. Hm. Would declaring a product dep on the Subsystem class itself be too awkward?

This comment has been minimized.

@benjyw

benjyw Dec 5, 2018

Contributor

Then you could only have one instance of the tool, even if you had multiple instances of the subsystem (scoped to multiple optionables).

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

The Subsystem class is abstract: if you wanted multiple instances, you'd define multiple subclasses?

This comment has been minimized.

@benjyw

benjyw Dec 5, 2018

Contributor

Backing up - We need something to serve as the key in the products map. That key needs to be unique per configured instance of the tool. I'm not sure what you're proposing as this key?

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

We have both a tool_subsystem_cls and a tool_instance_cls class. They are each subclasses of a different abstract interface. You've said that they need to be different interfaces so that different Tasks classes could use different tool_instance_cls subclasses.

I'm saying that if someone is going to have to subclass both the Subsystem base class and a tool class anyway, and the product is just a unique marker type, it's not clear why you need two interfaces. You could just use your Subsystem base class subclass as the marker.

This isn't a blocker... just trying to understand whether the split is actually useful/necessary.

This comment has been minimized.

@benjyw

benjyw Dec 6, 2018

Contributor

This is indeed nuanced. I will try and explain more clearly than I have been.

Say we have a tool Foo. There's one subclass of Subsystem called FooSubsystem (with options_scope foo) that encapsulates the requirements and entry point for Foo.

Now imagine that two unrelated tasks, Task1 and Task2, each need to run Foo. In general they may need different versions of Foo. With subsystems we achieve this idiomatically by using two different instances of FooSubsystem, each scoped to a different optionable. E.g., foo.task1, foo.task2.

Note that these are instances of the same class FooSubsystem. This allows us to configure them globally via the foo scope, and override each one, if necessary, in their subscopes (foo.task1, foo.task2). If we have two different subsystem subclasses (say with scopes foo_task1 and foo_task2) we lose this ability to have inner scopes inherit option values from the outer scope, and would have to configure each independently, even if the option values do not differ.

This is why it's not desirable for two different instances of the same tool to be represented by two different subclasses of Subsystem.

However we do still need two distinct keys in the product mapping. Hence the tool_instance_cls.

Does that make more sense?

@stuhood

stuhood approved these changes Dec 5, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Looks good to me!

Is this something that's primarily going to be used from inside pants' core, or would you expect plugin/contrib authors to interact with it? If so, it might be worth writing some tests around the integration points beyond the ones that cover isort's behavior.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 6, 2018

@baroquebobcat I'm planning to use it in a contrib (to run lambdex), so yeah, probably makes sense to test it independently of isort.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Dec 6, 2018

However I think I'd rather add the tests after I get that lambdex change done, so I can incorporate any learnings...

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Dec 6, 2018

That makes sense to me.

@benjyw benjyw merged commit c7b99ea into pantsbuild:master Dec 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:python_tool_base branch Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment