Unlock shebang++ #560

sourishkrout · 2024-04-19T16:19:46Z

Multiple things:

Allow cells to be store in ENV as long as their name conforms to an opinionated layout (we can give visual feedback in the notebook UI).
If a language is not executable and no interpreter/program is set, run cat to stash the input cell in the output.

This will enable use cases where e.g. you have a SQL query you're live-editing in one cell and running it with $ bq query (BigQuery CLI) in another cell. Enables better interplay between cells than just running them back-to-back ($__).

@adambabik wondering if we should deny overwriting variables already contained in the ENV? However, any export statement will let you do that 🤷. Also, I don't want to overload the Runner with too much "cell/block"-level terminology so I introduced known_id & known_name which are as below. The idea being that if a name or id is known, clients can specify them here. Otherwise they are "unknown" since they are not required to run programs. Open to suggestion to model this differently.

` ` `sql {"id":"01HT37XG04CWS4CQS2WS7P1MX2","name":"experiments_sql"}

PS: Also renamed StoreLastOutput to StoreEnvVars only in runnerv2 as a cleanup.

sourishkrout · 2024-04-19T16:29:00Z

internal/command/config_path_normalizer.go

@@ -52,6 +52,14 @@ func pathNormalizer(cfg *Config) (*Config, func() error, error) {
 		}
 	}

+	// default to "cat"
+	cat, err := exec.LookPath("cat")


@adambabik should this be using the new sys abstraction?

You can leave it as is. I need to port the system pkg usage to beta commands and runnerv2.

sourishkrout · 2024-04-19T16:38:05Z

Also just realized that runnerv2's command packages does not seem to have any TEMP_FILE test cases, no @adambabik?

adambabik · 2024-04-19T18:36:42Z

internal/api/runme/runner/v2alpha1/runner.proto

-  bool store_last_stdout = 23;
+  // store_env_vars, if true, will store the stdout under well known name
+  // and the last ran block in the environment variable `__`.
+  bool store_env_vars = 23;


Suggested change

bool store_env_vars = 23;

bool store_stdout_in_env = 23;

adambabik · 2024-04-19T18:43:06Z

internal/command/config_path_normalizer.go

@@ -52,6 +52,14 @@ func pathNormalizer(cfg *Config) (*Config, func() error, error) {
 		}
 	}

+	// default to "cat"
+	cat, err := exec.LookPath("cat")


You can leave it as is. I need to port the system pkg usage to beta commands and runnerv2.

adambabik · 2024-04-19T18:45:54Z

internal/command/config_path_normalizer.go

@@ -52,6 +52,14 @@ func pathNormalizer(cfg *Config) (*Config, func() error, error) {
 		}
 	}

+	// default to "cat"


Suggested change

// default to "cat"

// Default to "cat" when no program path is found.

// The idea is to return the body of the cell as its output

// so that it can be used as input in other cells.

adambabik · 2024-04-19T18:48:04Z

internal/runner/service.go

+		if knownName != "" && runnerConformsOpinionatedEnvVarNaming(knownName) {
+			err = sess.SetEnv(knownName, string(stdoutMem))
+			if err != nil {
+				logger.Sugar().Errorf("%v", err)


Suggested change

logger.Sugar().Errorf("%v", err)

logger.Warn("failed to set env", zap.Error(err))

In order to stay consistent.

adambabik · 2024-04-19T18:48:56Z

internal/runner/service.go

@@ -609,6 +618,12 @@ func runnerWinsizeToPty(winsize *runnerv1.Winsize) *pty.Winsize {
 	}
 }

+func runnerConformsOpinionatedEnvVarNaming(knownName string) bool {
+	// only allow uppercase letters, digits and underscores, min three chars
+	re := regexp.MustCompile(`^[A-Z_][A-Z0-9_]{1}[A-Z0-9_]*[A-Z][A-Z0-9_]*$`)


Suggestion: you can move this line outside of the func body to compile it only once.

adambabik · 2024-04-19T18:49:42Z

internal/runnerv2service/execution.go

-	session         *command.Session
-	storeLastStdout bool
+	session      *command.Session
+	storeEnvVars bool


Suggested change

storeEnvVars bool

storeStdoutInEnv bool

adambabik · 2024-04-19T18:49:51Z

internal/runnerv2service/execution.go

@@ -59,7 +61,7 @@ func newExecution(
 	id string,
 	cfg *command.Config,
 	session *command.Session,
-	storeLastStdout bool,
+	storeEnvVars bool,


Suggested change

storeEnvVars bool,

storeStdoutInEnv bool,

adambabik · 2024-04-19T18:51:24Z

internal/runnerv2service/execution.go

@@ -277,7 +283,7 @@ func (e *execution) closeIO() {
 }

 func (e *execution) storeLastOutput(r io.Reader) {


Suggested change

func (e *execution) storeLastOutput(r io.Reader) {

func (e *execution) storeOutputInEnv(r io.Reader) {

adambabik · 2024-04-19T19:01:23Z

internal/api/runme/runner/v1/runner.proto

@@ -177,6 +177,12 @@ message ExecuteRequest {

  // file extension associated with script
  string file_extension = 26;
+
+  // optional well known id for cell/block
+  string known_id = 27;


It seems that it is not used. We can keep it for the completeness reason.

The most important for me is to provide more detailed description. What does "known" mean? What guarantees does it give? Maybe given an example how client would use this.

Finally, if you take a look at config/v1alpha1/config.proto, we have a way to validate field values in the proto definition. This is a bit tricky and not used anywhere else, so it might be a better idea for a separate PR.

Will add an example. The reason why I added id (unused) was because I have near-term projects in mind where I need it. I'll be sure to make the description better. Going to skip validation in this PR for now.

adambabik · 2024-04-19T19:06:10Z

Also just realized that runnerv2's command packages does not seem to have any TEMP_FILE test cases, no @adambabik?

This is implemented in internal/command which is used in runnerv2. Check out command_args_normalizer.go which handles runnerv2alpha1.CommandMode_COMMAND_MODE_FILE. It is also tested (I checked code coverage) but implicitly.

adambabik · 2024-04-19T19:07:22Z

Also, I don't want to overload the Runner with too much "cell/block"-level terminology so I introduced known_id & known_name which are as below. The idea being that if a name or id is known, clients can specify them here. Otherwise they are "unknown" since they are not required to run programs. Open to suggestion to model this differently.

I think it's ok until we figure out a better approach. I would only ask for a more detailed description in the proto files. I added a comment in the source code.

sonarcloud · 2024-04-22T13:48:27Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
72.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sourishkrout requested a review from adambabik April 19, 2024 16:23

sourishkrout commented Apr 19, 2024

View reviewed changes

adambabik reviewed Apr 19, 2024

View reviewed changes

sourishkrout added 7 commits April 22, 2024 09:35

Baseline

9299cc6

Test storeLastStdout

87ac1e1

Use "known" terminology

ce1562b

Rename storeLastOutput to storeEnvVars in runnerv2

fce3b21

Port defaulting to cat to runnerv2

a9aab2f

Add nonexec cat default test case

fca4f34

Address review feedback

6aa4637

sourishkrout force-pushed the seb-sbpp branch from 221d9b1 to 6aa4637 Compare April 22, 2024 13:40

Remove merge leftover

f03f9e5

sourishkrout merged commit be6e4ce into main Apr 22, 2024
6 checks passed

sourishkrout deleted the seb-sbpp branch April 22, 2024 13:49

sourishkrout mentioned this pull request Apr 22, 2024

Unlock shebang++ stateful/vscode-runme#1303

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unlock shebang++ #560

Unlock shebang++ #560

sourishkrout commented Apr 19, 2024

sourishkrout Apr 19, 2024

adambabik Apr 19, 2024

sourishkrout commented Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

adambabik Apr 19, 2024

sourishkrout Apr 22, 2024 •

edited

Loading

adambabik commented Apr 19, 2024

adambabik commented Apr 19, 2024

sonarcloud bot commented Apr 22, 2024

-	// default to "cat"
+	// Default to "cat" when no program path is found.
+	// The idea is to return the body of the cell as its output
+	// so that it can be used as input in other cells.

	logger.Sugar().Errorf("%v", err)
	logger.Warn("failed to set env", zap.Error(err))

		@@ -277,7 +283,7 @@ func (e *execution) closeIO() {
		}

		func (e *execution) storeLastOutput(r io.Reader) {

	func (e *execution) storeLastOutput(r io.Reader) {
	func (e *execution) storeOutputInEnv(r io.Reader) {

Unlock shebang++ #560

Unlock shebang++ #560

Conversation

sourishkrout commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourishkrout Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

adambabik commented Apr 19, 2024

adambabik commented Apr 19, 2024

sonarcloud bot commented Apr 22, 2024

Quality Gate passed

sourishkrout Apr 22, 2024 •

edited

Loading