Implement SPICE-0009 External Readers #660

HT154 · 2024-09-23T18:52:49Z

SPICE-0009

Includes the changes from #640

Adoption of this feature in language binding libraries:

pkl-server/src/test/kotlin/org/pkl/server/MessagePackCodecTest.kt

bioball

Did a first pass.

This is looking pretty good! Looks pretty polished, with just a couple of issues.

Design question: what do we want to do about env vars or CWD of the process?

What should users expect if thay do:

pkl --env-var foo=bar --external-module-reader ldap=my-ldap-reader

Should my-ldap-reader receive foo=bar as an env var? Or should there be some way to configure the env vars that get passed to this reader?

Same question applies for --working-dir.

Thoughts: probably not appropriate to pass these values into the process, but it should be clearly documented. But, we should probably have some way to configure env var, as well as CWD for these processes.

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt

pkl-commons/src/main/kotlin/org/pkl/commons/Strings.kt

pkl-commons/src/test/kotlin/org/pkl/commons/ShlexTest.kt

pkl-commons/src/main/kotlin/org/pkl/commons/Strings.kt

pkl-core/src/main/java/org/pkl/core/externalProcess/ExternalProcessImpl.java

bioball · 2024-10-14T21:15:54Z

pkl-server/src/main/kotlin/org/pkl/server/Server.kt

@@ -226,6 +256,9 @@ class Server(private val transport: MessageTransport) : AutoCloseable {
 if (message.clientModuleReaders?.isNotEmpty() == true) {
 add(ClientModuleKeyFactory(message.clientModuleReaders, transport, evaluatorId))
 }
+ for ((scheme, spec) in message.externalModuleReaders ?: emptyMap()) {
+ add(ModuleKeyFactories.external(scheme, getExternalProcess(evaluatorId, spec), evaluatorId))


Do these external readers ever need to care about what the evaluator ID is?

Not really, since the MessageTransport is distinct from the server's. One reason to still populate this is PKL_DEBUG logging. Because the message contents are printed, this could help debug issues in multi-evaluator scenarios.

stdlib/EvaluatorSettings.pkl

This is preparatory work for [SPICE-0009](apple/pkl-evolution#10). It is being contributed in a separate pull request to ease review. The Message, Message(En|De)coder, and MessageTransport types have been ported to Java and moved to a new `org.pkl.core.messaging` package.

[SPICE-0009](apple/pkl-evolution#10) New close flow

Box Bytes, fix error CLI handling during project load Bytes equals compare

Co-authored-by: Daniel Chao <daniel.h.chao@gmail.com> Apply suggestions from code review Co-authored-by: Daniel Chao <daniel.h.chao@gmail.com> Rename --external-* CLI flags to --external-*-reader Rename ExternalProcess -> ExternalReaderProcess Add org.pkl.core.util.Readers.closeQuietly and deprecate ModuleReaders.closeQuietly Clean up doc comments Refactor ExternalReaderRuntime into pkl-config-java Refactor out External*Resolver classes, rename ResourceReaders.ExternalDelegate and ModuleKeys.External to MessageTransport

bioball

One more pass. Getting close!

pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java

pkl-config-java/src/main/java/org/pkl/config/java/ExternalReaderRuntime.java

pkl-core/src/main/java/org/pkl/core/externalReader/package-info.java

pkl-core/src/main/java/org/pkl/core/messaging/Message.java

pkl-core/src/main/java/org/pkl/core/messaging/Messages.java

pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java

bioball · 2024-10-17T23:22:49Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java

@@ -771,4 +777,56 @@ public boolean hasElement(SecurityManager securityManager, URI elementUri)
 return projectResolver.getResolvedDependenciesForPackage(packageUri, dependencyMetadata);
 }
 }
+
+ public static class MessageTransport implements ModuleKey {


Suggested change

public static class MessageTransport implements ModuleKey {

public static class External implements ModuleKey {

Hmm, I had renamed this to align with ResourceReaders.MessageTransport since, like that class, this one is used by both external readers and client readers. I think I'd prefer to keep this name and preserve the symmetry. Thoughts?

Ooh I see.

Maybe ResourceReaders.ExternalResolver?

And then call the other one ResourceReaders.ExternalProcess?

And then apply the same names on the module side?

Done! I also moved the External*Resolver classes into the module/resource packages since they're not specific to the external reader stuff. I can also see reason for putting these in messaging, so let me know if you'd prefer that.

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java

pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java

stdlib/EvaluatorSettings.pkl

bioball

Overall LGTM! Great work!

This also needs eyes from @holzensp @stackoverflow because it touches the core language.

…al resolvers into module/resource packages

HT154 commented Sep 23, 2024

View reviewed changes

pkl-server/src/test/kotlin/org/pkl/server/MessagePackCodecTest.kt Show resolved Hide resolved

HT154 force-pushed the external-readers branch 4 times, most recently from 337ab5a to 9d8a587 Compare September 24, 2024 15:53

HT154 force-pushed the external-readers branch 2 times, most recently from f6d8810 to 19bc962 Compare October 3, 2024 03:54

HT154 marked this pull request as ready for review October 3, 2024 03:57

HT154 mentioned this pull request Oct 6, 2024

Support for future objects #669

Closed

HT154 force-pushed the external-readers branch 9 times, most recently from a935e9d to 9251522 Compare October 11, 2024 05:50

HT154 mentioned this pull request Oct 13, 2024

SPICE-0009: External Readers apple/pkl-evolution#10

Open

bioball requested changes Oct 14, 2024

View reviewed changes

HT154 force-pushed the external-readers branch 3 times, most recently from 4fc48da to 98ec98f Compare October 16, 2024 00:35

HT154 requested a review from bioball October 16, 2024 00:39

HT154 added 2 commits October 17, 2024 13:07

Split server-specific messages/encoding/decoding back into pkl-server

87016a7

HT154 force-pushed the external-readers branch 2 times, most recently from 9d35221 to 97313c9 Compare October 17, 2024 20:22

HT154 and others added 2 commits October 17, 2024 13:29

Implement SPICE-0009 External Readers

477b454

[SPICE-0009](apple/pkl-evolution#10) New close flow

docs

b9c7a19

HT154 added 2 commits October 17, 2024 13:29

Add ExternalReaderRuntime, lots of test coverage

6920775

Add doc comments to new public APIs

bac0086

HT154 force-pushed the external-readers branch from 97313c9 to 9323690 Compare October 17, 2024 20:29

HT154 and others added 2 commits October 17, 2024 13:32

Improve backcompat for new language binding libs on old pkl versions

60da0f9

Box Bytes, fix error CLI handling during project load Bytes equals compare

HT154 force-pushed the external-readers branch from 9323690 to e561cc3 Compare October 17, 2024 20:33

bioball requested changes Oct 17, 2024

View reviewed changes

HT154 added 2 commits October 17, 2024 23:07

Address review feedback

20f111d

Use records for message types

01db901

HT154 requested a review from bioball October 18, 2024 06:36

Update message type codes

e4403d1

bioball approved these changes Oct 18, 2024

View reviewed changes

Update naming for resource readers, module key + factory; Move extern…

f5ba7c7

…al resolvers into module/resource packages

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SPICE-0009 External Readers #660

Implement SPICE-0009 External Readers #660

HT154 commented Sep 23, 2024 •

edited

Loading

bioball left a comment

bioball Oct 14, 2024

HT154 Oct 15, 2024

bioball left a comment

bioball Oct 17, 2024

HT154 Oct 18, 2024

bioball Oct 18, 2024

HT154 Oct 18, 2024

bioball left a comment

	public static class MessageTransport implements ModuleKey {
	public static class External implements ModuleKey {

Implement SPICE-0009 External Readers #660

Are you sure you want to change the base?

Implement SPICE-0009 External Readers #660

Conversation

HT154 commented Sep 23, 2024 • edited Loading

bioball left a comment

Choose a reason for hiding this comment

bioball Oct 14, 2024

Choose a reason for hiding this comment

HT154 Oct 15, 2024

Choose a reason for hiding this comment

bioball left a comment

Choose a reason for hiding this comment

bioball Oct 17, 2024

Choose a reason for hiding this comment

HT154 Oct 18, 2024

Choose a reason for hiding this comment

bioball Oct 18, 2024

Choose a reason for hiding this comment

HT154 Oct 18, 2024

Choose a reason for hiding this comment

bioball left a comment

Choose a reason for hiding this comment

HT154 commented Sep 23, 2024 •

edited

Loading