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

Memoize stable task creation #5654

Merged
merged 2 commits into from Apr 4, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Apr 4, 2018

Problem

After Goal.clear(), task subclasses created for the same superclasses and scopes will not be equal/identical to those from before Goal.clear().

As part of its maintenance of the task/goal lifecycle, pantsd clears the goals, which currently causes the creation of multiple non-identical but otherwise equal subclasses, and that caused me some confusion on #5639 where I was attempting to use task classes as dict keys.

Solution

Memoize creation of the stable task subclasses (similar to the strategy used in Collection.of).

Result

Task classes behave as expected in #5639.

Memoize creation of stable task subclasses to avoid ending up with se…
…parate instances for separate runs of `Goal`.

@stuhood stuhood requested review from jsirois , benjyw and kwlzn Apr 4, 2018

@jsirois

jsirois approved these changes Apr 4, 2018

@stuhood stuhood force-pushed the twitter:stuhood/memoize-stable-task-creation branch from d9cd10d to 2adf57c Apr 4, 2018

@kwlzn

kwlzn approved these changes Apr 4, 2018

Copy link
Member

kwlzn left a comment

lgtm

@benjyw

benjyw approved these changes Apr 4, 2018

task1_pre, = install()
Goal.clear()
task1_post, = install()
self.assertEquals(task1_pre, task1_post)

This comment has been minimized.

@benjyw

benjyw Apr 4, 2018

Contributor

Do you want to check that they have the same id? Or is semantic equality all we actually care about anyway?

This comment has been minimized.

@stuhood

stuhood Apr 4, 2018

Member

Semantic equality is really what matters, yea. Although in theory I could/should have checked __hash__ as well.

@stuhood stuhood merged commit 899fb73 into pantsbuild:master Apr 4, 2018

1 check failed

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

@stuhood stuhood deleted the twitter:stuhood/memoize-stable-task-creation branch Apr 4, 2018

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