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

Run node in a different execution environment #11173

Merged
merged 27 commits into from
Oct 9, 2024

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Sep 25, 2024

Pull Request Description

close #10719

Changelog:

  • add: optional expressionConfigs parameter to the executionContext/recompute request
  • update: IdExecutionInstrument allowing to run a single node in a specified execution environment
  • refactor: move tests related to the recompute request to a separate test suite. Otherwise the RuntimeServerTest is becoming too bloated

Important Notes

The updated executionContext/recompute request.

interface ExecutionContextRecomputeParameters {
  /** The execution context identifier. */
  contextId: ContextId;

  /** The expressions that will be invalidated before the execution.
   *
   *  Only the provided expression ids are invalidated excluding the dependencies.
   */
  invalidatedExpressions?: "all" | ExpressionId[];

  /** The execution environment that will be used in the execution. */
  executionEnvironment?: ExecutionEnvironment;

  /** The execution configurations for particular expressions.
   *
   *  The provided expressions will be invalidated from the cache with the
   *  dependencies. The result of the execution will stay in the cache until the
   *  cache is invalidated by editing the node or other means.
   */
  expressionConfigs?: ExpressionConfig[];
}

interface ExpressionConfig {
  /** The expression identifier. */
  expressionId: ExpressionId;
  /** The execution environment that should be used to run this expression. */
  executionEnvironment?: ExecutionEnvironment;
}

Use cases

  • to re-run a single node without re-running the dependent nodes (subtree), put the node id in the invalidatedExpressions parameter.
  • to re-run a node with dependent nodes (subtree), put the node id in the expressionConfigs parameter with empty executionEnvironment
  • to re-run a node in a different execution environment, put the node id in the expressionConfigs and specify the executionEnvieronment

Checklist

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

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 25, 2024
@4e6 4e6 self-assigned this Sep 25, 2024
@4e6 4e6 force-pushed the wip/db/10719-run-node-in-a-different-context branch from 3d11bac to 169e386 Compare September 26, 2024 11:22
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.

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

val invalidatedExpressions = builder.result()
if (invalidatedExpressions.nonEmpty) {
val updates = invalidatedExpressions.collect {
case expressionId if expressionId ne null =>
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the null condition even possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember I added this guard to fix NPE. But I don't remember exactly how the null value appears

/**
* @return the root of this node.
*/
public abstract RootNode getRootNode();
Copy link
Member

@JaroslavTulach JaroslavTulach Oct 4, 2024

Choose a reason for hiding this comment

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

No, we do not want to expose getRootNode(). It is only used by:

EnsoContext context = EnsoContext.get(info.getRootNode());
originalExecutionEnvironment = context.getExecutionEnvironment();

and corresponding setter. Just call it getExecutionEnvironment() and setExecutionEnvironment.

Btw. There already is setExceptionEnvironment below why we have two ways of doing the same thing? Can you document the methods and describe different among them?

@JaroslavTulach JaroslavTulach self-requested a review October 4, 2024 15:30
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.

I don't like the statefulness kept inside of the IdExecutionService. Rather than that let the only service only suggest a change of environment by having a method like:

StringOrSomeEnvironmentType findExecutionEnvironment(Info);

and then let all the work of enabling it and especially disabling it in the IdExecutionInstrument itself.

That will simplify the API to a single method and we can remove getRootNode and other new methods.

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.

The IdExecutionService interface with single method addition looks fine. Few more comments on the engine&instrument side.

var iteration = 0
while (!isProgramStarted && iteration < 100) {
val out = context.consumeOut
Thread.sleep(200)
Copy link
Member

Choose a reason for hiding this comment

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

Ech. Thread.sleep doesn't belong into programs and it is better to avoid them in tests too. Please replace with proper wait/notify primitives:

context.waitForOut(200);

that will be notified immediately as soon as some output is delivered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output is updated from the Enso program. How do you notify the waiting method in the test?

@@ -87,10 +88,31 @@ public final class EnsoLanguage extends TruffleLanguage<EnsoContext> {
private static final LanguageReference<EnsoLanguage> REFERENCE =
LanguageReference.create(EnsoLanguage.class);

private final ContextThreadLocal<ExecutionEnvironmentReference> threadExecutionEnvironment =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this variable in the EnsoLanguage and not in EnsoContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Oct 9, 2024
@mergify mergify bot merged commit 78993a0 into develop Oct 9, 2024
41 of 42 checks passed
@mergify mergify bot deleted the wip/db/10719-run-node-in-a-different-context branch October 9, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to run a single node in a different context in engine.
4 participants