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

feat:declare optional schema on streamsync state #278

Conversation

FabienArcellier
Copy link
Collaborator

@FabienArcellier FabienArcellier commented Mar 2, 2024

implement #277

Schema declaration on the Application state allows Streamsync to handle complex serialization scenario and empower your IDE and toolchains to provide autocomplete and type checking.

I will create another PR to handle compute property.


This PR fix #226

without schema, the button is working only one time

Peek 2024-03-04 19-48

with schema

Peek 2024-03-04 19-51


Peek 2024-03-04 19-24


Peek 2024-03-02 11-42

@FabienArcellier FabienArcellier changed the base branch from master to dev March 2, 2024 10:31
@FabienArcellier FabienArcellier force-pushed the 277-declare-optional-schema-on-streamsync-state-1 branch 8 times, most recently from 407bd7a to cb6f3d9 Compare March 2, 2024 12:05
@FabienArcellier FabienArcellier marked this pull request as draft March 3, 2024 07:36
>>>
>>> initial_state = new_initial_state(MyState)
"""
global initial_state
Copy link
Collaborator Author

@FabienArcellier FabienArcellier Mar 4, 2024

Choose a reason for hiding this comment

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

Rewriting a module variable there introduces a risk of introducing futur bugs with current accessors. When a variable is import from external module, its value is freezed. There is a risk to replace the value of the variable without updating external module.

To reduce the risk of issue, I would advocate to expose a function that return the variable initial_state(). Using it, we remove the risk of freezing the value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, keeping the global variable but using an accessor rather than importing it directly?

Like this?

def get_initial_state():
  return initial_state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes exactly

@FabienArcellier FabienArcellier force-pushed the 277-declare-optional-schema-on-streamsync-state-1 branch from cb6f3d9 to c95a645 Compare March 4, 2024 18:34
@FabienArcellier FabienArcellier marked this pull request as ready for review March 4, 2024 20:00
@@ -0,0 +1,123 @@
# State schema

Schema declaration on the [Application state](./application-state) allows Streamsync to handle complex serialization
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some minor comments about style, e.g. British spelling, capitalisation of some words, etc, but I'll take care of them. Nice docs though!

@@ -204,6 +203,9 @@ def ingest(self, raw_state: Dict) -> None:
for key, raw_value in raw_state.items():
self.__setitem__(key, raw_value)

def items(self) -> Sequence[Tuple[str, Any]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this? Is this necessary or a separate improvement to make it look more like a Dict?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need a way to iterate through keys in state proxy in the test test_replace_dictionary_content_in_a_state_with_schema_should_transform_it_in_state_proxy_and_trigger_mutation

items = _state['items']
items = {k: v for k, v in items.items() if k != "Apple"}
_state["items"] = items

I may remove this method and doing something like that :

items = _state['items']
items = {k: v for k, v in items..to_dict().items() if k != "Apple"}
_state["items"] = items

Copy link
Contributor

@mmikita95 mmikita95 left a comment

Choose a reason for hiding this comment

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

Potentially reintroduces #171; clarification needed.


# Then
m = _state._state_proxy.get_mutations_as_dict()
assert m == {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a problem with this: it counts 'initial' mutations (i.e. first call to the get_mutations_as_dict) well, but can't track them after it.

Consider this example:

# When
items = _state['items']
m = _state._state_proxy.get_mutations_as_dict()
# Initial mutation
assert m == {
    '+items': None,
    '+items.Apple': None,
    '+items.Apple.name': 'Apple',
    '+items.Apple.type': 'fruit',
    '+items.Cucumber': None,
    '+items.Cucumber.name': 'Cucumber',
    '+items.Cucumber.type': 'vegetable',
    '+items.Lettuce': None,
    '+items.Lettuce.name': 'Lettuce',
    '+items.Lettuce.type': 'vegetable'
} # True
items = {k: v for k, v in items.items() if k != "Apple"}
_state["items"] = items

# Then
m = _state._state_proxy.get_mutations_as_dict()
# equals to {'+items': None}
# which will nullify the "items" on frontend
# should have been {'-items.Apple': None}
# or {
#    '+items': None,
#    '+items.Cucumber': None,
#    '+items.Cucumber.name': 'Cucumber',
#    '+items.Cucumber.type': 'vegetable',
#    '+items.Lettuce': None,
#    '+items.Lettuce.name': 'Lettuce',
#    '+items.Lettuce.type': 'vegetable'
#} 

Our biggest challenge with this are nested structures (lists and dicts), as we can only see the upper level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @mmikita95. There was something wrong. I set some changes trying to fix it. Could you take a look ?

value = raw_value

self.state[key] = value
self.state[key] = raw_value
Copy link
Contributor

Choose a reason for hiding this comment

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

In regard to the previous comment - these StateProxy conversions/reinitializations were in place to avoid this kind of issue (though, not accounting for nested lists). Could you give a little more insight on motivation behind removing this, so I could better understand on how we should proceed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have to move StateProxy creation in the State because, it's this object that know the schema. You should take a look on the method State._set_state_item.

@FabienArcellier FabienArcellier force-pushed the 277-declare-optional-schema-on-streamsync-state-1 branch from 1af34cd to e15f581 Compare March 5, 2024 20:29
@raaymax
Copy link
Collaborator

raaymax commented Mar 6, 2024

I see that tests for state propagation proven to be useful :D

    Expected string: "{\"a\":1,\"b\":2,\"c\":{\"d\":3}}"
    Received string: "{\"a\":1,\"b\":2,\"c\":null}"

@FabienArcellier FabienArcellier force-pushed the 277-declare-optional-schema-on-streamsync-state-1 branch 5 times, most recently from fad8524 to 620f410 Compare March 12, 2024 21:24
@FabienArcellier FabienArcellier force-pushed the 277-declare-optional-schema-on-streamsync-state-1 branch from 620f410 to 677be06 Compare March 18, 2024 15:18
@ramedina86 ramedina86 merged commit f95fb7e into writer:dev Mar 18, 2024
16 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change on value of a graph as vega spec does not trigger mutation in extension
4 participants