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 parallel batch mode to scripted tests #3151

Merged
merged 31 commits into from May 10, 2017

Conversation

Projects
None yet
5 participants
@jvican
Copy link
Member

commented Apr 28, 2017

This pull request has two goals:

  • Speed up scripted tests by allowing them to run in parallel.
  • Allow scripted to run scripted tests by reusing the same sbt instance.

However, these tasks ended up not being easy. This is why this PR also addresses other issues, like the ones described in #3160.

The integrity of the tests has not been changed, except for one: auto-plugins, which still tests all the behaviour of plugins but uncovered #3164. As a result, I've done a temporary fix that accounts for the fact that reload in the batch mode will modify the value being tested by one.

This PR also deletes some deprecated methods in scripted and adjusts the code (both for scripted sbt and scripted plugin) accordingly.

@eed3si9n eed3si9n added the ready label Apr 28, 2017

@jvican jvican force-pushed the scalacenter:parallel-scripted-tests branch 2 times, most recently from 27e7881 to 2e2eecf Apr 28, 2017

@dwijnand
Copy link
Member

left a comment

LGTM.

But I expect Travis CI to fail because those deprecations are more tricky than just removing them - that's why I had left them alone for now.

You might want to just drop those hunks to get this green and merged.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

Oh, and well done, thank you very much for this!

@eed3si9n
Copy link
Member

left a comment

scripted forks a new instance of sbt, so I don't know if this would work on Travis CI containers due to limited memory.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

scripted forks a new instance of sbt, so I don't know if this would work on Travis CI containers due to limited memory.

@eed3si9n You're right. This brings me to the second improvement that I wanted to make: reusing sbt instances to run things in parallel.

This is a shame since tests are running much faster locally. The overhead of sbt scripted tests is mainly I/O -- this is why parallelizing them yields profitable gains.

Honestly, I think that Travis is not enough for sbt. I would be happy to put sbt/sbt under the Scala Center CI, where we have 64 cores and 14GB of memory. Would you be happy with it? /cc @dwijnand

@jvican

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2017

I have pushed a commit that reduces the memory for every scripted test, in the hope that this makes Travis happy... let's see.

@jvican

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2017

No luck, unfortunately. I'll try to make my next improvement to see if that helps us get out of this situation. Otherwise, I would suggest to change CIs, at this point it's clear that Travis is a blocker...

@@ -80,7 +80,10 @@ final class SbtHandler(directory: File,
val thread = new Thread() { override def run() = { p.exitValue(); server.close() } }

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 29, 2017

Member
[error] x compiler-project / inc-pickled-refinement compiler-project / inc-pickled-refinement
[error]  Cause of test exception: {line 7}  Command failed: Remote sbt initialization failed

The forked sbt on the other side is failing to get up, so you might need more println around here where the Process is being spawned.

This comment has been minimized.

Copy link
@jvican

jvican Apr 29, 2017

Author Member

Thanks for the tip.

I think the issue here is memory since all these tests are passing locally. Can we discuss migrating CI to a more powerful machine, either Drone (free) or paying for a more powerful Travis?

We need to find a good way to move forward.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Apr 30, 2017

Member

We can potentially discuss it, but I'm relatively happy with Travis CI as we get 5 workers for free. I think that's pretty good ROI.

This comment has been minimized.

Copy link
@jvican

jvican Apr 30, 2017

Author Member

You get 5 workers for free but the turnaround time is at minimum 45 minutes for every commit you do, and there's usually a long queue. We need more powerful alternatives. I will not insist on using our free CI, but can you please consider paying a pro account in Travis to increase the CI resources?

Being honest here, contributing to sbt is less than an ideal experience. I've tried solving it by parallelizing the testing engine, but even if that is blocked by Travis resources, I don't know what else to do. In my view, there is no other way to move forward than increasing the resources.

@jvican jvican force-pushed the scalacenter:parallel-scripted-tests branch 2 times, most recently from 3587188 to 58a7ba6 May 1, 2017

.

@jvican jvican changed the title Make scripted tests parallel Add parallel batch mode to scripted tests May 2, 2017

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

@dwijnand @eed3si9n This is ready for review. Tests are passing and description is updated. I will report on the benefits brought by this PR tomorrow.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 2, 2017

Note to myself: I have yet to update to latest librarymanagement, but I'll do that in another PR.

@eed3si9n
Copy link
Member

left a comment

Overall looks pretty good.
Posted a few minor questions for clarification.

val options = "-cp" :: s"$defaultClasspath" :: "-Yrangepos" :: Nil
val reportError = (msg: String) => globalReporter.error(NoPosition, msg)
val reportError = (msg: String) => System.err.println(msg)

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 2, 2017

Member

We get into trouble when we start printlning directly. I am guessing this is to surface scripted failures, but would this have negative impact to normal usage?

This comment has been minimized.

Copy link
@jvican

jvican May 2, 2017

Author Member

No, not at all. In fact, this should never happen. It happened only in this case because I was fiddling with the classpath of the compiler. But this should not affect normal users.


// Run the test and delete files (except global that holds local scala jars)
val result = runOrHandleDisabled(label, tempTestDir, runTest, buffer)
IO.delete(tempTestDir.*("*" -- "global").get)

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n May 2, 2017

Member

This wipes out the forked instances temp directory? So you're saving time by not resolving sbt 1.0.0-SNAPSHOT from the second batch onwards?

This comment has been minimized.

Copy link
@jvican

jvican May 2, 2017

Author Member

Yes, this wipes out all the content of the temp directory except the global dir holding the scala jars. The benefits of running scripted in batch more are several, though -- for instance, you always keep a hot sbt instance around.

@jvican jvican force-pushed the scalacenter:parallel-scripted-tests branch 4 times, most recently from 4660bdf to 8210090 May 3, 2017

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

After playing around with lots of options, this PR is now finished.

Improvements:

  • Batch mode execution.
  • Parallel execution (by default 1 sbt instance because Travis doesn't allow more).
  • Forked sbt compilation and scripted run.
  • Tests in #1360 fixed.

It is a large PR (in number of commits), but line changes are small. I suggest reviewing all at once, and if there's a concrete question you can check if the commit message that introduced a change explains it.

As a result, and even with only one sbt instance running scripted tests, this PR speeds up the scripted tests by almost 2x.

I believe we may win a little bit more of a time when util-X11 is merged, which brings parallel ivy downloads.

In the future, when performance issues and memory leaks are addressed in sbt, I will come back to scripted to try to run 2 sbt instances at a time.

In the meanwhile, if you wanna run scripted locally, I suggest that you change the number of sbt instances here. It does bring a speedup.

@Duhemm
Copy link
Contributor

left a comment

Awesome improvements, very impressive!

  • Why has the test project-load/sha-conflict moved from pending to disabled?
  • Travis CI kills a build if it doesn't produce any output for 10 minutes. It appears that people circumvent that issue by disabling log buffering (hopefully they'll never have to do that again, thanks to that PR 🎉). Would it be possible to have scripted periodically write something next to the test name, to show that it's not dead? Something like
Running run / hello-world .............. done!
val globalValue = (demo in Global).value
same(globalValue, "global 0", "demo in Global")
same(globalValue, "global 1", "demo in Global") // this is temporary, should be 0 until # is fixed

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 3, 2017

Contributor

Until what is fixed?

This comment has been minimized.

Copy link
@jvican

jvican May 3, 2017

Author Member

$ copy-file changes/taskAppendN/build.sbt build.sbt
-> compile
-> reload

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 3, 2017

Contributor

So scripted was starting up a whole new sbt between every command???

This comment has been minimized.

Copy link
@jvican

jvican May 3, 2017

Author Member

What I meant is that sbt was initialized lazily, only when > statements were invoked. compile here implied initializing sbt.

checkIterations := {
val expected: Int = (Space ~> NatBasic).parsed
val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
assert(expected == actual, s"Expected $expected compilations, got $actual")

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 3, 2017

Contributor

Shouldn't this test be in zinc's suite?

This comment has been minimized.

Copy link
@jvican

jvican May 3, 2017

Author Member

I have to port these improvements to Zinc, indeed! The issue is that this has to be included in every build.sbt. This is why we have to put it in here.

checkIterations := {
val expected: Int = (Space ~> NatBasic).parsed
val actual: Int = ((compile in Compile).value match { case a: Analysis => a.compilations.allCompilations.size }) - CompileState.previousIterations
assert(expected == actual, s"Expected $expected compilations, got $actual")

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 3, 2017

Contributor

This one should be moved to, shouldn't it? I think zinc's scripted supports this kind of assertion.

There are certainly other tests that could be moved over to zinc. Is there a reason to keep them here?

This comment has been minimized.

Copy link
@jvican

jvican May 3, 2017

Author Member

All these tests should be moved to Zinc yes. But I'd like to do that in the future, I don't have enough cycles for it now.

@@ -22,7 +32,7 @@ TaskKey[Unit]("checkCompilations") := {
val files = fileNames.map(new java.io.File(_))
assert(recompiledFiles(iteration) == files, "%s != %s".format(recompiledFiles(iteration), files))
}
assert(allCompilations.size == 2)
assert(allCompilations.size == 2, s"All compilations is ${allCompilations.size}")

This comment has been minimized.

Copy link
@Duhemm

Duhemm May 3, 2017

Contributor

Maybe say that we expected 2?

@eed3si9n

This comment has been minimized.

Copy link
Member

commented May 3, 2017

It is a large PR (in number of commits), but line changes are small. I suggest reviewing all at once, and if there's a concrete question you can check if the commit message that introduced a change explains it.

Once the review is done, could we squash them into 1 or several number of commits for future debugging purpose?

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@eed3si9n Honestly, I prefer not to. They have been done this way so that they are easy to reason about, and each one implements a granular change that may be useful for the future. That being said, I would like to squash those commits that have to do with travis options and "disable parallelism".

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Travis CI kills a build if it doesn't produce any output for 10 minutes. It appears that people circumvent that issue by disabling log buffering (hopefully they'll never have to do that again, thanks to that PR 🎉). Would it be possible to have scripted periodically write something next to the test name, to show that it's not dead?

This would be an excellent issue for a Scala Center spree, thanks for the idea!

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

Can we try to get this in? When reviews are done, I'll squash the pertinent commits. I'd like to get this in first before making more PRs.

jvican added some commits May 1, 2017

Add batch mode execution to scripted
For that, we:

* Change the existing infrastructure to recycle as much code as
  possible.
* Use `BatchScriptRunner` since `ScriptRunner` is too restrictive to
  programmatically control the underlying sbt servers.
* Unify `TestRunner` to the more general way for both batch and
  non-batch modes.
Use scripted hook to run reload
This saves us from duplicating error handling logic to correctly manage
`pending` tests whose projects don't even load (and make our reload)
fail.
Fix compile tests using `checkIterations`
Those tests using `checkIterations` were not working because an
invariant was working: the first iterations number is 0. As we reuse
sbt for running several tests, this is no longer true, so this commit
makes sure that they are updated with the pertinent infrastructure to
compute the performed iterations from the latest known number of
iterations.
Fix java options for scripted
* Remove MaxPermSize from another scripted opts
* Reduce memory of sbt host to 1g instead of 2g
* Add Xms java options to scripted
* Enable parallelism with 512M for sbt tests
* Parallelism + batch mode
Fix fancy uses of analysis
Clean up first analysis to make sure that test utilities using
information from analysis have correct information that does not collide
with the compilation analysis of the previous runs (batch tests).
Fix #3610: project/old-ops
`compile` does nothing in this test, what we need to do is `reload`.
Fix project/binary-plugin: Reload was implied
The first `>` that appears kickstarts the execution of sbt, so it
implies a reload. This is no longer true for the batch test execution.
Set name of root project if no name is known
This commit makes sure that between every run in batch mode the local
root project name is updated if no name is set.

This fixes project/generated-root-no-project that was assuming that the
root directory name was `generated-root-no-project`. This invariant does
not hold anymore with the batch-mode. Now one sbt dir is shared for lots
of scripted tests.
Create a compiler plugin for scripted
The compiler plugin is more appropriate than running a series of
commands because of the effects of `set`.
Fix auto-plugins temporarily until #3163 is fixed
Unfortunately, I think this uncovers a bug in duplicated computation of
settings in the `ThisBuild` and `Global` axis.
Fix string-to-command to ignore existing commands
The scripted batch executor injects a command and the operation
performed in this test checks all the commands, assuming only `noop` is
declared.
Try another travis structure + build tweaks
I've tried to put together some scripted tests to remove the overhead of
compiling the whole sbt, which is around 3 minutes every time.

This new structure *should* make the scripted tests run faster.

Aside from this, we do some more tweaks:

* Increase memory.
* Fork processes to compile and run (to see if it makes a difference).
* Pass in the server flag to sbt.

@jvican jvican force-pushed the scalacenter:parallel-scripted-tests branch from 1134b61 to f967f6c May 10, 2017

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

Can someone reload this failed build instance? https://travis-ci.org/sbt/sbt/jobs/230899635 The culprit is #3170.

The rest of the tests are passing.

@jvican

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

Tests are passing.

@eed3si9n eed3si9n merged commit a265600 into sbt:1.0.x May 10, 2017

1 check passed

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

@eed3si9n eed3si9n removed the ready label May 10, 2017

@muuki88

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

@jvican great work! Sbt-native-packager scripted test run for almost 40 minutes. This will be a huge development experience improvement 😍😍

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.