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

[rsc-compile] bump version; rm metacp jobs; update tests #7272

Merged
merged 6 commits into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@baroquebobcat
Copy link
Contributor

commented Feb 21, 2019

Rsc has been updated so that it can consume regular classpaths in all cases, eliminating the need to run metacp before and after rsc. This removes all the extra jobs. It is based on @cosmicexplorer's #7227.

The compile portions are ready for a first pass of review, but I still need to do more work on the tests.

Travis will fail on it right now, because the newer Rsc version hasn't been pushed to maven central yet.

It depends on #7268, but has no additional changes to zinc_compile.py

@stuhood
Copy link
Member

left a comment

Thanks!

@cosmicexplorer
Copy link
Contributor

left a comment

I now realize that writing the test cases at each step is a great reason alone to split up the mixed compile changes. There are a couple points which you were probably already planning to add some comments to, and I dumped my perception of the dependency graph changes into a review comment in case that was helpful. I will be making a followup ticket now to move all of the distribution selection logic into jvm_compile.py.

output_products=[
runtime_classpath_product,
self.context.products.get_data('rsc_classpath')],
dep_keys=only_zinc_invalid_dep_keys(invalid_dependencies))),

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Feb 21, 2019

Contributor

This ends up silently losing rsc parallelism for all the zinc-only scala targets (which is going to be the vast majority of targets), which is why I was thinking splitting the enum up into -java and -scala versions was a good idea. zinc-only targets still only depend on the rsc compiles of their dependencies -- except for targets with any java sources, since javac can't yet depend on rsc output.

As per https://github.com/pantsbuild/pants/pull/7227/files#diff-074dd871ff62c835ab90d4a0a1264a63R314 or https://github.com/pantsbuild/pants/pull/7227/files#diff-074dd871ff62c835ab90d4a0a1264a63R549: if we're electing to use this coarser strategy to avoid introducing the intermediate zinc_scala_classpath_from_rsc product in this diff, I would want an extremely clear TODO describing:

  • what parallelism we're dropping (all non-rsc targets are waiting for the zinc compiles of all their dependencies).
  • what changes to the dependency graph must be made in followups.

The end result of the mixed compile changes in terms of the execution graph and intermediate products was documented in several comments in #7227 and I'm concerned about losing that and mysteriously losing parallelism later if the performance implications of the dependency graph of the zinc jobs isn't documented here.

This is of course the most complex part of the whole thing and will be changing a lot, and imho this would become significantly more maintainable if we spent a small amount of time converting to v2 rules and letting the v2 engine construct the graph implicitly.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Feb 22, 2019

Contributor

As far as this PR is concerned, though, zinc-only targets == targets with java, right? So is this comment something to address when that stops being the case, rather than speculatively here?

I'm slightly confused, though, about the javac part of this comment, and how it relates to this line of code, which could maybe do with a clarifying code comment. As far as I know, we never invoke javac (we use zinc to compile java), and zinc is always happy with jars output by rsc. With that understanding, we could use the rsc produced output on the classpath here. So could you add a comment explaining which part of my understanding is false, and why we need to use the output from zinc?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Feb 22, 2019

Contributor

As far as this PR is concerned, though, zinc-only targets == targets with java, right? So is this comment something to address when that stops being the case, rather than speculatively here?

I did not realize this, see:

for all the zinc-only scala targets (which is going to be the vast majority of targets)

so this review comment can largely be ignored. I would love at least to have a comment here describing why we are setting up the dependencies like we are right now (explaining that rsc cannot produce jars to satisfy javac yet -- unless that's 100% false)...

As far as I know, we never invoke javac (we use zinc to compile java), and zinc is always happy with jars output by rsc.

I was under the impression zinc invoked javac because it was consistently mentioned that producing jars to satisfy javac was an rsc TODO. If that was only a concern for warp because it doesn't use zinc, then my misunderstanding here is almost definitely the reason for any performance regression in the previous mixed compile branch.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Feb 26, 2019

Author Contributor

This ends up silently losing rsc parallelism for all the zinc-only scala targets (which is going to be the vast majority of targets), which is why I was thinking splitting the enum up into -java and -scala versions was a good idea.

Since this review doesn't include mixed compile support, zinc only compiles of targets with just scala that are not tests isn't supported, so I'm not sure this applies.

zinc-only targets still only depend on the rsc compiles of their dependencies -- except for targets with any java sources, since javac can't yet depend on rsc output.

I think you're saying that the zinc-only workflow should use rsc output of their dependencies, apart from java source having targets. I didn't do that here, because I was trying to keep this change to just removing metacp as much as possible. I'd like to have adding additional workflows be separate patches so that their effects on behavior are clearer.

The current workflows are

  • rsc then zinc: eg the current rsc-compatible scala flow. Where rsc jobs depend on rsc and zinc jobs depend on the rsc of the current jobs dependencies
  • zinc only: each target is compiled by zinc and requires zinc artifacts on the classpath. Currently this applies to java containing targets and tests.

When we add mixed compile support, we could add another workflow

  • zinc with rsc dependencies: for scala targets that fail to compile with rsc, but can build against rsc generated jars.

I was under the impression zinc invoked javac
Yes. It does. Zinc is just a wrapper for Javac and Scalac. It adds some behavior on top, in particular doing analysis and supporting incremental builds using that.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Feb 28, 2019

Contributor

I'm great with adding the workflow specified when introducing mixed compile support!

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py Outdated
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
@illicitonion
Copy link
Contributor

left a comment

This looks great :) The code reads a lot more clearly in this factoring of methods - thanks for cleaning it up so much!

output_products=[
runtime_classpath_product,
self.context.products.get_data('rsc_classpath')],
dep_keys=only_zinc_invalid_dep_keys(invalid_dependencies))),

This comment has been minimized.

Copy link
@illicitonion

illicitonion Feb 22, 2019

Contributor

As far as this PR is concerned, though, zinc-only targets == targets with java, right? So is this comment something to address when that stops being the case, rather than speculatively here?

I'm slightly confused, though, about the javac part of this comment, and how it relates to this line of code, which could maybe do with a clarifying code comment. As far as I know, we never invoke javac (we use zinc to compile java), and zinc is always happy with jars output by rsc. With that understanding, we could use the rsc produced output on the classpath here. So could you add a comment explaining which part of my understanding is false, and why we need to use the output from zinc?

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated
@baroquebobcat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

TODO. I need to add the rsc(current) -> zinc_against_rsc(current) edge back in because leaving it out causes issues with v1 build cache contents. Namely, zinc may run first, and cache the zinc outputs without the rsc ouputs.
Or, we could add a new node that runs after both rsc(current)and zinc_against_rsc(current) that just updates the v1 cache.
Or or, we could drop the in progress cache updates.

baroquebobcat added some commits Feb 21, 2019

[rsc-compile] bump version; rm metacp jobs; update tests
Rsc has been updated so that it can consume regular classpaths in all cases, eliminating the need to run metacp before and after rsc. This removes all the extra jobs. It is based on @cosmicexplorer's #7227.

The compile portions are ready for a first pass of review, but I still need to do more work on the tests.

Travis will fail on it right now, because the newer Rsc version hasn't been pushed to maven central yet.

@baroquebobcat baroquebobcat force-pushed the twitter:nhoward/rsc_metacpless branch from 1f1ca53 to 2e71a82 Feb 26, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Or or, we could drop the in progress cache updates.

This would not be great, as it would mean that compiles that failed to compile anything would fail to cache everything. Would make a bunch of CI use cases more expensive.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Yeah. It's not a realistic way to fix it.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

I went with the approach of adding the zinc_against_rsc(A) -> rsc(A) edges back since that changes behavior the least.

@cosmicexplorer
Copy link
Contributor

left a comment

Thanks for synthesizing the multitude of comments! I am great with waiting to introduce rsc compiles for dependencies of zinc-only targets until mixed compile lands.

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
output_products=[
runtime_classpath_product,
self.context.products.get_data('rsc_classpath')],
dep_keys=only_zinc_invalid_dep_keys(invalid_dependencies))),

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Feb 28, 2019

Contributor

I'm great with adding the workflow specified when introducing mixed compile support!

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py
}""").strip(),
dependee_graph)

def test_scala_lib_with_java_sources_not_passed_to_rsc(self):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Feb 28, 2019

Contributor

Great test!

@baroquebobcat baroquebobcat merged commit 1dd8207 into pantsbuild:master Feb 28, 2019

1 check passed

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

cosmicexplorer added a commit that referenced this pull request Feb 28, 2019

Split out rsc distribution selection into JvmCompile (#7290)
### Problem

See #7272 (comment). We are selecting the jvm distribution in a pretty manual way in `rsc_compile.py` that might benefit from being extracted into the `JvmCompile` base class.

### Solution

- Move `_get_jvm_distribution()` from `RscCompile` into `JvmCompile`.
- Use that logic to simplify `ZincCompile#_get_zinc_arguments()`.

### Result

Distribution selection, and in particular preparing the distribution to be usable in a hermetic context, is extracted into a common method in the jvm compile base class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.