Variables view feature #297

nikolay-egorov · 2021-07-06T16:02:37Z

No description provided.

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/Notebook.kt

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableState.kt

ileasile · 2021-07-08T12:06:21Z

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableState.kt

+data class VariableState(
+ val propertyKlass: KProperty1<Any, *>,
+ val scriptInstance: Any,
+ var stringValue: String


It should be val, at least here, in API class. You may make an interface with val stringValue and then implement it in this class.

So we basically would like to consider that VariableState is an immutable thing, hence, from the semantic perspective, changing stringValue gives us a new VariableState? Sure thing, I can have another var field in a implementation

Basically, property class and script instance hold the exhaustive information about variable state. Other things may be obtained from these two. So I suggest something like this:

data class VariableState( private val propertyClass: KProperty1<Any, *>, private val scriptInstance: Any, ) { // caching, methods to get pure and stringified values go here }

What do you think?

Cool, I did this way :

interface VarState { val propertyKlass: KProperty<*> val scriptInstance: Any? val stringValue: String? } data class VariableState( override val propertyKlass: KProperty1<Any, *>, override val scriptInstance: Any, var mutableStringValue: String? ) : VarState { override val stringValue: String? get() = mutableStringValue }

But I like your approach more

...iler/src/main/kotlin/org/jetbrains/kotlinx/jupyter/compiler/util/serializedCompiledScript.kt

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl/impl/InternalEvaluatorImpl.kt

src/main/kotlin/org/jetbrains/kotlinx/jupyter/util.kt

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/ReplTests.kt

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/testUtil.kt

ileasile · 2021-07-09T15:51:02Z

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/Notebook.kt

+ /**
+ * Current state of visible variables
+ */
+ val variablesMap: Map<String, VariableState>


Let's call it variablesState

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/Notebook.kt

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableState.kt

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl/impl/InternalEvaluatorImpl.kt

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/ReplTests.kt

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/CustomLibraryResolverTests.kt

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/IntegrationApiTests.kt

html report generation

…ences

…nd refactor

ileasile

Almost all is OK. The latest serious thing which I don't like is that stringified values are exposed as a part of Notebook API. They may be exposed, but at least with the corresponding raw values.

ileasile · 2021-07-13T10:55:11Z

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableStateImpl.kt

@@ -0,0 +1,29 @@
+package org.jetbrains.kotlinx.jupyter.api


Let's rename this class to VariableState.kt

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableStateImpl.kt

ileasile · 2021-07-13T11:38:51Z

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/VariableStateImpl.kt

+import kotlin.reflect.jvm.isAccessible
+
+interface VariableState {
+ val propertyKlass: KProperty<*>


What about renaming it to property or kProperty? I don't see a reason for a word class

src/main/kotlin/org/jetbrains/kotlinx/jupyter/htmlUtil.kt

ileasile · 2021-07-13T11:45:51Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/htmlUtil.kt

+ return buildString {
+ append(
+ """
+ <table class="$varsTableStyleClass" style="width:80%;margin-left:auto;margin-right:auto;" align="center">


Something happened with indentation here, I think that additional tab will make things better

ileasile · 2021-07-13T11:49:37Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl.kt

+ if (isHtmlFormat) notebook.variablesReportAsHTML() else notebook.variablesReport()
+ )
+
+ // TODO: causes some tests to fail. perhaps, due to exhausting log calling


What tests are failing? Let's do something with them

ileasile · 2021-07-13T11:53:46Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl.kt


- EvalResult(rendered, EvaluatedSnippetMetadata(newClasspath, compiledData, newImports))
+ notebook.updateVariablesState(variablesState)
+ notebook.cellVariables = cellVariables


Let's create a Notebook method that will be called like notebook.updateVariablesState(internalEvaluator). It seems that we can make cellVariables a val property

It seems like we cant make it val since we need to reasign it once it changed

src/main/kotlin/org/jetbrains/kotlinx/jupyter/util.kt

ileasile · 2021-07-13T12:25:48Z

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/ReplTests.kt

+ assertEquals(res.metadata.evaluatedVariablesState, varsUpdate)
+ assertFalse(repl.notebook.variablesState.isEmpty())
+ val varsState = repl.notebook.variablesState
+ assertEquals("1", varsState["x"]!!.stringValue)


Let's add extension method that will allow us to write varsState.getStringValue("x"). And also, varsState.getValue("x") for getting raw value

ileasile · 2021-07-13T15:17:12Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl.kt

 newImports = importsCollector.popAddedImports()
 }
+ val variablesState = internalEvaluator.variablesHolder
+ val cellVariables = internalEvaluator.cellVariables


Is this variable needed?

ileasile · 2021-07-13T15:17:36Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl.kt

 compiledData = internalEvaluator.popAddedCompiledScripts()
 newImports = importsCollector.popAddedImports()
 }
+ val variablesState = internalEvaluator.variablesHolder


This variable may be inlined

ileasile · 2021-07-13T15:18:48Z

jupyter-lib/api/src/main/kotlin/org/jetbrains/kotlinx/jupyter/api/Notebook.kt

+ * Value: set of variable names.
+ * Useful <==> declarations + modifying references
+ */
+ var cellVariables: Map<Int, Set<String>>


Do we still need it to be a var?

We might create another field and make val in api. I think it might make sense since we do not want to allow change notebook.varsState from notebook

Let's do so then

ileasile · 2021-07-13T15:19:58Z

src/main/kotlin/org/jetbrains/kotlinx/jupyter/repl/impl/InternalEvaluatorImpl.kt

+ if (property is KMutableProperty1) {
+ variablesHolder.remove(property.name)
+ }
+
+ if (property !is KMutableProperty1) {
+ variablesHolder[property.name] = state


if-else?

Could you expand a bit more?

ileasile · 2021-07-13T15:22:42Z

src/test/kotlin/org/jetbrains/kotlinx/jupyter/test/repl/AbstractReplTest.kt

+ protected fun Map<String, VariableState>.getStringValue(variableName: String): String? {
+ return get(variableName)?.stringValue
+ }
+
+ protected fun Map<String, VariableState>.getValue(variableName: String): Any? {
+ return get(variableName)?.value
+ }
+


Let's replace these declarations to testUtil.kt, they may be useful in other tests as well

ileasile · 2021-07-13T18:42:31Z

Thank you @nikolay-egorov! Merged manually as c6b1390

nikolay-egorov requested a review from ileasile July 6, 2021 16:02

nikolay-egorov self-assigned this Jul 6, 2021

nikolay-egorov force-pushed the variables-view-feature branch 2 times, most recently from 922f348 to b2400c7 Compare July 7, 2021 14:05

ileasile requested changes Jul 8, 2021

View reviewed changes

ileasile requested changes Jul 9, 2021

View reviewed changes

nikolay-egorov added 4 commits July 12, 2021 19:03

Command :vars added, Notebook.kt stores vars state

236428c

Added VariableState.kt as a meta-storage, so String -> VariableState;

9da6cb5

html report generation

Add simple cache proto

2538d8e

Variables usage per cell info added: declarations and modifying refer…

fef40d4

…ences

nikolay-egorov force-pushed the variables-view-feature branch from 8ab2741 to 76f1d75 Compare July 12, 2021 16:11

nikolay-egorov added 2 commits July 12, 2021 19:15

Made InternalEvaluator stateless and improved refactor; some tests added

3e8c2ec

Add support for private/protected/internal vars; Make better naming a…

0b1fdcd

…nd refactor

nikolay-egorov force-pushed the variables-view-feature branch 2 times, most recently from b203e86 to 58d0a74 Compare July 12, 2021 16:36

Fix HTML table report; encapsulate logic about cell variables

58d0a74

nikolay-egorov requested a review from ileasile July 13, 2021 06:32

ileasile reviewed Jul 13, 2021

View reviewed changes

Add value property to VariableState; minor renaming

6ec1281

nikolay-egorov force-pushed the variables-view-feature branch from b7fbc83 to 6ec1281 Compare July 13, 2021 16:02

ileasile closed this Jul 13, 2021

ileasile deleted the variables-view-feature branch July 14, 2021 19:49

Uh oh!

Variables view feature #297

Variables view feature #297

Uh oh!

Conversation

nikolay-egorov commented Jul 6, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolay-egorov Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ileasile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

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

Choose a reason for hiding this comment

ileasile commented Jul 13, 2021

Labels

2 participants

nikolay-egorov Jul 9, 2021 •

edited

Loading