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

Fix incremental compilation of runtime/test #8975

Merged
merged 41 commits into from Feb 13, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Feb 5, 2024

Fixes #8973 and #9026

Pull Request Description

Fixes incremental compilation issues of runtime/Test by introducing a separate runtime-integration-tests project. Also move runtime benchmarks to a new runtime-benchmarks project and provide an easier way how to run the benchmarks.

For now, let's move all the tests from runtime/Test to runtime-integration-tests, just to be sure that all the tests run with all the dependencies. In the future, we can add standard unit tests, that do not require the Enso runtime, to runtime/Test.

Important Notes

  • Introduced runtime-benchmarks project
    • Contains all benchmarks previously from runtime/bench.
    • Refactored some Scala bits to Java
  • Introduced runtime-integration-tests project
    • Contains all tests previously from runtime/test.
  • Quality of life improvements to the benchmark running.
    • bench and benchOnly commands work as they used to.
    • All source code is under src/main/java, no more bench directory.
    • There is a main class that delegates to the JMH launcher, you can use run -h to see all the available cmd line options for the launcher.
    • No more RegressionTest.scala.
    • See the updated benchmarks.md.
  • Simplification of std-benchmarks project
    • Remove the Bench configuration
    • Use JPMSPlugin.

How I tested the changes?

Incremental compilation of runtime-tests tested by:

  • Modification of sources in pkg, runtime, and runtime-instrument-common and then re-running runtime-tests/testOnly.

Tested other commands:

  • sbt:runtime-integration-tests> withDebug --debugger
  • sbt:runtime-benchmarks> run - i 1 -wi 1 org.enso.interpreter.bench.benchmarks.semantic.AtomBenchmarks.benchSumList

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Make sure there are no regressions in the engine benchmarks

@Akirathan Akirathan self-assigned this Feb 5, 2024
@@ -114,8 +114,6 @@ object JPMSPlugin extends AutoPlugin {
)
}.toSeq

ensureDirectoriesExist(modulePath, log)
Copy link
Member Author

Choose a reason for hiding this comment

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

These warnings are annoying and they are not useful. Java does not mandate that the directories passed into, e.g., --module-path option exist.

@Akirathan Akirathan force-pushed the wip/akirathan/8968-fix-runtime-test-build branch from c8e91a3 to 7e934d9 Compare February 5, 2024 17:08
@Akirathan Akirathan force-pushed the wip/akirathan/8968-fix-runtime-test-build branch from c496ae9 to 0997cf2 Compare February 7, 2024 15:32
@Akirathan Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Feb 7, 2024
@@ -49,6 +49,17 @@ public void resetOutput() {
out.reset();
}

@Test
public void jvmAssertionsAreEnabled() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I have spent ridiculous amount of time debugging RuntimeErrorsTest just to realize that I have not enabled JVM assertions, so the DataFlow errors did not contain stack traces. Let's add this test so that nobody encounters something similar in the future.

Copy link
Contributor

@mwu-tow mwu-tow left a comment

Choose a reason for hiding this comment

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

Reviewed Rust parts only. 🟢

@Akirathan Akirathan force-pushed the wip/akirathan/8968-fix-runtime-test-build branch from ec3f437 to 5d57cc2 Compare February 8, 2024 15:56
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Hard to review such a huge shuffling of code, but the intentions I have spotted are good.

@@ -1650,109 +1617,6 @@ lazy val runtime = (project in file("engine/runtime"))
necessaryModules ++ langs ++ tools
}
)
.settings(
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping the setup of runtime project will be simpler. Removal of these lines matches my expectations.

* the `org.enso.runtime` JPMS module, so it is easier to keep them in a separate project.
* For standard unit tests, use `runtime/Test`.
*/
lazy val `runtime-integration-tests` =
Copy link
Member

Choose a reason for hiding this comment

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

Configuration of this project remains complicated, but at the end, they are integration tests - they can be complicated.

@Akirathan Akirathan linked an issue Feb 12, 2024 that may be closed by this pull request
@@ -1869,7 +1869,7 @@ lazy val `runtime-benchmarks` =
(Compile / run).toTask("").tag(Exclusive).value
}
.dependsOn(
`runtime-fat-jar` / assembly
buildEngineDistribution
Copy link
Member

Choose a reason for hiding this comment

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

+1

@Akirathan Akirathan force-pushed the wip/akirathan/8968-fix-runtime-test-build branch from 596e07e to f873840 Compare February 12, 2024 15:23
@Akirathan Akirathan added the p-high Should be completed in the next sprint label Feb 12, 2024
@Akirathan
Copy link
Member Author

Akirathan commented Feb 12, 2024

Scheduling engine benchmarks at https://github.com/enso-org/enso/actions/runs/7873980506/job/21482523287, and stdlib benchmarks at https://github.com/enso-org/enso/actions/runs/7873983053

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.
GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

build.sbt Show resolved Hide resolved
@Akirathan
Copy link
Member Author

Akirathan commented Feb 12, 2024

Encountered some problems with bench-report.xml uploading in the last bench runs. Starting a new engine bench at https://github.com/enso-org/enso/actions/runs/7875921777/job/21488889969

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan Akirathan merged commit 5919eda into develop Feb 13, 2024
27 of 29 checks passed
@Akirathan Akirathan deleted the wip/akirathan/8968-fix-runtime-test-build branch February 13, 2024 09:05
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

LGTM (sorry for late review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. p-high Should be completed in the next sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "Benchmark Engine" daily job Incremental compilation of runtime/test is broken
6 participants