Add Serializable Vars State with nested fields descriptors #314

nikolay-egorov · 2021-07-22T14:04:24Z

No description provided.

ileasile

I didn't look through all the code, but I have some meta-comments.
Also, it seems that serialization may be performance-expensive, so let's introduce a flag (with the value from a system property and defaulting to true) that turns off the serialization (empty map should be sent in metadata)

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

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

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

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

nikolay-egorov · 2021-07-27T06:47:04Z

empty map should be sent in metadata

Maybe not empty, but just one-depth stringed values as it was before?

ileasile · 2021-07-27T08:01:47Z

I think that empty is better. If the client does not support this functionality, it will not benefit from any data we'll send in this map.

nikolay-egorov · 2021-07-27T17:31:26Z

Wandering through the park, I came across this. We could completely go for jvm fields instead of KProperties since there are some cases when we need jvms

ileasile

I have some more comments. Still not finished the review

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

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

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

ileasile · 2021-08-20T17:15:09Z

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

+    }
+}
+
+class ReplVarsSerializationTest : AbstractSingleReplTest() {


Let's move it to another file... And maybe refactor it somehow to make less lines of code?

ileasile · 2021-08-20T17:20:11Z

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

+    private val unchangedVariables: MutableSet<V> = mutableSetOf()
+
+    fun removeOldDeclarations(address: K, newDeclarations: Set<V>) {
+        // removeIf?


Please review all comments in the code. If they don't have an explaining function, let's remove them

ileasile · 2021-08-20T17:20:29Z

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

+                variablesDeclarationInfo.remove(it)
+                unchangedVariables.remove(it)
+            }
+//            predicate


Same thing about comments

ileasile · 2021-08-20T17:38:21Z

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

-    LIST_ERRORS_REPLY(ListErrorsReply::class);
+    LIST_ERRORS_REPLY(ListErrorsReply::class),
+
+    SERIALIZATION_REQUEST(SerializationRequest::class),


I think that it's a bad name for this type of request. Maybe VARIABLES_VIEW_REQUEST? Serialization is implementation detail

ileasile · 2021-08-23T16:02:00Z

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

+                return cellSet.key
+            }
+        }
+        return -1


Maybe we should return null here instead of -1?

ileasile · 2021-08-24T15:31:04Z

build.gradle.kts

@@ -12,6 +12,8 @@ plugins {

 val deploy: Configuration by configurations.creating

+val serializationFlagProperty = "jupyter.serialization.enabled"


Let's just inline this property

…ences

…riptor name, not cellID

… properly

…iptors right away, not through 'size' and 'data' fields

…es from cache; some improvements

ileasile

Please be more attentive when fix issues. Most of them are global, and should be fixed not only in one place but all across the code

ileasile · 2021-12-07T03:45:20Z

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

+@Serializable
+data class SerializableTypeInfo(val type: Type = Type.Custom, val isPrimitive: Boolean = false, val fullType: String = "") {
+    companion object {
+        val ignoreSet = setOf("int", "double", "boolean", "char", "float", "byte", "string", "entry")


Это поле не используется нигде. Если что-то используется только в плагине, надо написать тесты на то, как это там используется. Если не используется нигде, удалить

ileasile · 2021-12-07T03:45:45Z

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

+    companion object {
+        val ignoreSet = setOf("int", "double", "boolean", "char", "float", "byte", "string", "entry")
+
+        val propertyNamesForNullFilter = setOf("data", "size")


Аналогично (unused)

ileasile · 2021-12-07T03:53:13Z

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

+            val isPrimitive = !(
+                if (fullType != "Entry") (isContainer ?: false)
+                else true
+                )
+


fullType != "Entry" isContainer Result
true true false
true false or null true
false true false
false false or null false

Это таблица для AND, значит всё это выражение можно упростить до isPrimitive = fullType!="Entry" && isContainer != true

ileasile · 2021-12-07T03:54:28Z

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

+                else true
+                )
+
+            return SerializableTypeInfo(enumType, isPrimitive, fullType)


Зачем нам тип передавать в двух видах? Честно говоря, выглядит стрёмно

ileasile · 2021-12-07T03:56:29Z

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

+
+@Serializable
+enum class Type {
+    Map,


Капсом лучше писать

ileasile · 2021-12-07T12:12:03Z

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

@@ -149,6 +152,11 @@ class NotebookImpl(
    override val jreInfo: JREInfoProvider
        get() = JavaRuntime

+    fun updateVariablesState(evaluator: InternalEvaluator) {


It seems like a very internal method. We most likely don't want to expose it to user

ileasile · 2021-12-07T12:18:04Z

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

+            jupyterId = 3
+        )
+        state = repl.notebook.unchangedVariables
+        // tmp disable to further investigation (locally tests pass on java8)


It also passes for me locally. I think ignoring is a bad solution

ileasile · 2021-12-07T12:20:09Z

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

+            jupyterId = 1
+        )
+        var state = repl.notebook.unchangedVariables
+        state.size.shouldBe(3)


Actually, what do you test here? What variables should be contained in this list? What if this test fail, how will you understand what variables have changed without debugging?

ileasile · 2021-12-07T12:20:54Z

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

+            """.trimIndent(),
+            jupyterId = 1
+        )
+        var state = repl.notebook.unchangedVariables


Also, why do you need this variable? It's useless

ileasile · 2021-12-07T12:25:43Z

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

@@ -318,4 +321,121 @@ class ReplVarsTest : AbstractSingleReplTest() {
        varState.getStringValue("x") shouldBe "25.0"
        varState.getValue("x") shouldBe 25.0
    }
+
+    @Test
+    fun testAnonymousObjectRendering() {


This is the test from other file that was deleted

nikolay-egorov requested a review from ileasile July 22, 2021 14:04

nikolay-egorov self-assigned this Jul 22, 2021

nikolay-egorov force-pushed the serializable-vars-state branch 2 times, most recently from 434f8e4 to 54cc8ed Compare July 24, 2021 21:00

nikolay-egorov linked an issue Jul 26, 2021 that may be closed by this pull request

Variables descriptors #316

Open

ileasile reviewed Jul 26, 2021

View reviewed changes

nikolay-egorov force-pushed the serializable-vars-state branch 4 times, most recently from 90c8b39 to e050cb6 Compare July 28, 2021 14:31

ileasile reviewed Aug 2, 2021

View reviewed changes

nikolay-egorov force-pushed the serializable-vars-state branch from 1a5a0c4 to 8cfc15e Compare August 3, 2021 10:02

nikolay-egorov force-pushed the serializable-vars-state branch 3 times, most recently from c14a522 to 961a320 Compare August 12, 2021 08:22

nikolay-egorov force-pushed the serializable-vars-state branch 6 times, most recently from 5820ba6 to 39aa087 Compare August 20, 2021 16:01

ileasile force-pushed the serializable-vars-state branch from bd39684 to 001534c Compare September 13, 2021 18:00

nikolay-egorov force-pushed the serializable-vars-state branch 4 times, most recently from f16ffa0 to f347d0f Compare September 14, 2021 18:06

ileasile reviewed Sep 30, 2021

View reviewed changes

nikolay-egorov force-pushed the serializable-vars-state branch 2 times, most recently from 17ffdfd to d0d5a1d Compare November 25, 2021 15:17

nikolay-egorov added 24 commits December 6, 2021 11:03

Add incremental serialization support prototype; support cyclic refer…

eb38537

…ences

Add variable serializer to repl state; fixed some tests

e72e266

Consider anonymous classes; still need a bit of to do

f3bbb28

Make RunTimeWrapper

0131e41

Add new message type for vars serialization

79bebea

Change main reflection to jvm Fields; use KReflection for built-ins

e9aefaa

Add possibility to use serialization request with only top-level desc…

e5fac02

…riptor name, not cellID

Get unchangedVariables from notebook to reflect after execution state…

f3a3ce1

… properly

Add support for set container; make standard Arrays have values descr…

dc305f7

…iptors right away, not through 'size' and 'data' fields

Add suspended cache validation; Add possibility to remove old variabl…

18e844c

…es from cache; some improvements

Prevent complete cyclic serialization on server side

42751f2

Fix top level recursive serialization

445583e

Add ID for serialized objects

a00fcec

Consider changes in objects via immutable refs

0355749

Use custom lazy delegate

b4ac2ce

Store all meta-data related to a top-level variable name

7ac866a

Delete meta-data of redeclared variables

d6f6d39

Improve skeleton lists handling

4ddb8ea

Remove serialization cache

f9a67d5

Fix recursion detection

ec951f7

Make serialization contain comm_id to respect jupyter comm handlers

5aebd3d

Add special type classes in shared-compiler

c1b2344

Sync with master, do some refactor

2a1081a

Tmp fix one strangely falling test

ef7df1e

nikolay-egorov force-pushed the serializable-vars-state branch from d0d5a1d to ef7df1e Compare December 6, 2021 08:19

ileasile requested changes Dec 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Serializable Vars State with nested fields descriptors #314

Add Serializable Vars State with nested fields descriptors #314

nikolay-egorov commented Jul 22, 2021

ileasile left a comment

nikolay-egorov commented Jul 27, 2021 •

edited

ileasile commented Jul 27, 2021

nikolay-egorov commented Jul 27, 2021

ileasile left a comment

ileasile Aug 20, 2021

ileasile Aug 20, 2021

ileasile Aug 20, 2021

ileasile Aug 20, 2021

ileasile Aug 23, 2021

ileasile Aug 24, 2021

ileasile left a comment

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

ileasile Dec 7, 2021

		@@ -12,6 +12,8 @@ plugins {

		val deploy: Configuration by configurations.creating

		val serializationFlagProperty = "jupyter.serialization.enabled"

Add Serializable Vars State with nested fields descriptors #314

Are you sure you want to change the base?

Add Serializable Vars State with nested fields descriptors #314

Conversation

nikolay-egorov commented Jul 22, 2021

ileasile left a comment

Choose a reason for hiding this comment

nikolay-egorov commented Jul 27, 2021 • edited

ileasile commented Jul 27, 2021

nikolay-egorov commented Jul 27, 2021

ileasile left a 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 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolay-egorov commented Jul 27, 2021 •

edited