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.

Sign up for GitHub

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

Feature/#521 refactor code for databricks integration #2284

Merged
merged 9 commits into from
Dec 3, 2024
Merged

Feature/#521 refactor code for databricks integration #2284

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
309fceb
refactor data converter for databricks topic
Nov 15, 2024
a8c8396
refactor checker code
Nov 27, 2024
7996e5f
minor fix
Nov 27, 2024
f986964
minor fix
Nov 27, 2024
3611990
refactor check properties type
Nov 28, 2024
41a7a60
attempt to fix failing tests on python 3.9
Nov 28, 2024
03fe9a6
attempt to fix failing tests on python 3.9
Nov 28, 2024
be1d263
Update taipy/core/config/checkers/_data_node_config_checker.py
toan-quach Dec 3, 2024
0e445b9
fixed failed test
Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 42 additions & 24 deletions taipy/core/config/checkers/_data_node_config_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# specific language governing permissions and limitations under the License.

from datetime import timedelta
from typing import Dict, List, cast
from typing import Any, Callable, Dict, List, Tuple, cast

from taipy.common.config._config import _Config
from taipy.common.config.checker._checkers._config_checker import _ConfigChecker
Expand All @@ -23,6 +23,27 @@


class _DataNodeConfigChecker(_ConfigChecker):
_PROPERTIES_TYPES: Dict[str, List[Tuple[Any, List[str]]]] = {
DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: [
(
Callable,
[
DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY,
DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY,
],
)
],
DataNodeConfig._STORAGE_TYPE_VALUE_SQL: [
(
Callable,
[
DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY,
DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY,
],
),
],
}
Comment on lines +27 to +45
Copy link
Member

Choose a reason for hiding this comment

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

  1. We should also add all the properties to check. not only the callable.

  2. The dictionary should be moved close to the definition of the properties, in the DataNodeCOnfig class.

  3. The format seems strange to me. I would have used a Dict[str, Dict[str, Any]] instead of Dict[str, List[Tuple[Any, List[str]]]]

    {
    DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: 
        {
            DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY: Callable,
            DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY: Callable
        },
    DataNodeConfig._STORAGE_TYPE_VALUE_SQL:
        {
            DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY: Callable,
            DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY: Callable
        }
}

    What do you think? Is there a reason to group the properties by type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sure, well don't have much reason haha. Since the original code focused mainly on Callable, I just went along with that :D I have created a ticket to resolve this if you're okay with that since the changes are bigger than what we're having atm. https://github.com/orgs/Avaiga/projects/6/views/2?pane=issue&itemId=89479596

Copy link
Member

Choose a reason for hiding this comment

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

Great. Thx!


def __init__(self, config: _Config, collector: IssueCollector):
super().__init__(config, collector)

Expand All @@ -46,7 +67,7 @@ def _check(self) -> IssueCollector:
self._check_scope(data_node_config_id, data_node_config)
self._check_validity_period(data_node_config_id, data_node_config)
self._check_required_properties(data_node_config_id, data_node_config)
self._check_callable(data_node_config_id, data_node_config)
self._check_class_type(data_node_config_id, data_node_config)
self._check_generic_read_write_fct_and_args(data_node_config_id, data_node_config)
self._check_exposed_type(data_node_config_id, data_node_config)
return self._collector
Expand Down Expand Up @@ -196,28 +217,25 @@ def _check_generic_read_write_fct_and_args(self, data_node_config_id: str, data_
f"DataNodeConfig `{data_node_config_id}` must be populated with a Callable function.",
)

def _check_callable(self, data_node_config_id: str, data_node_config: DataNodeConfig):
properties_to_check = {
DataNodeConfig._STORAGE_TYPE_VALUE_GENERIC: [
DataNodeConfig._OPTIONAL_READ_FUNCTION_GENERIC_PROPERTY,
DataNodeConfig._OPTIONAL_WRITE_FUNCTION_GENERIC_PROPERTY,
],
DataNodeConfig._STORAGE_TYPE_VALUE_SQL: [
DataNodeConfig._REQUIRED_WRITE_QUERY_BUILDER_SQL_PROPERTY,
DataNodeConfig._OPTIONAL_APPEND_QUERY_BUILDER_SQL_PROPERTY,
],
}

if data_node_config.storage_type in properties_to_check.keys():
for prop_key in properties_to_check[data_node_config.storage_type]:
prop_value = data_node_config.properties.get(prop_key) if data_node_config.properties else None
if prop_value and not callable(prop_value):
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a Callable function.",
)
def _check_class_type(self, data_node_config_id: str, data_node_config: DataNodeConfig):
if data_node_config.storage_type in self._PROPERTIES_TYPES.keys():
for class_type, prop_keys in self._PROPERTIES_TYPES[data_node_config.storage_type]:
for prop_key in prop_keys:
prop_value = data_node_config.properties.get(prop_key) if data_node_config.properties else None
if prop_value and not isinstance(prop_value, class_type):
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a {'Callable' if class_type == Callable else class_type.__name__}.",
)
if class_type == Callable and callable(prop_value) and prop_value.__name__ == "<lambda>":
self._error(
prop_key,
prop_value,
f"`{prop_key}` of DataNodeConfig `{data_node_config_id}` must be"
f" populated with a Callable and not a lambda.",
toan-quach marked this conversation as resolved.
Show resolved Hide resolved
)

def _check_exposed_type(self, data_node_config_id: str, data_node_config: DataNodeConfig):
if not isinstance(data_node_config.exposed_type, str):
Expand Down
38 changes: 23 additions & 15 deletions taipy/core/data/_data_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from copy import copy
from datetime import datetime, timedelta
from pydoc import locate
from typing import Dict

from .._repository._abstract_converter import _AbstractConverter
from ..common._utils import _load_fct
Expand Down Expand Up @@ -120,14 +121,13 @@ def __serialize_exposed_type(properties: dict, exposed_type_key: str, valid_str_
for v in properties[exposed_type_key]
]
else:
properties[
exposed_type_key
] = f"{properties[exposed_type_key].__module__}.{properties[exposed_type_key].__qualname__}"
properties[exposed_type_key] = (
f"{properties[exposed_type_key].__module__}.{properties[exposed_type_key].__qualname__}"
)
return properties

@classmethod
def _entity_to_model(cls, data_node: DataNode) -> _DataNodeModel:
properties = data_node._properties.data.copy()
def _serialize_properties(cls, data_node: DataNode, properties: Dict) -> Dict:
if data_node.storage_type() == GenericDataNode.storage_type():
properties = cls.__serialize_generic_dn_properties(properties)

Expand All @@ -144,6 +144,11 @@ def _entity_to_model(cls, data_node: DataNode) -> _DataNodeModel:
properties = cls.__serialize_exposed_type(
properties, cls._EXPOSED_TYPE_KEY, cls._VALID_STRING_EXPOSED_TYPES
)
return properties

@classmethod
def _entity_to_model(cls, data_node: DataNode) -> _DataNodeModel:
properties = cls._serialize_properties(data_node, data_node._properties.data.copy())

return _DataNodeModel(
data_node.id,
Expand Down Expand Up @@ -273,25 +278,28 @@ def __deserialize_exposed_type(properties: dict, exposed_type_key: str, valid_st
return properties

@classmethod
def _model_to_entity(cls, model: _DataNodeModel) -> DataNode:
data_node_properties = model.data_node_properties.copy()

def _deserialize_properties(cls, model: _DataNodeModel, properties: Dict) -> Dict:
if model.storage_type == GenericDataNode.storage_type():
data_node_properties = cls.__deserialize_generic_dn_properties(data_node_properties)
properties = cls.__deserialize_generic_dn_properties(properties)

if model.storage_type == JSONDataNode.storage_type():
data_node_properties = cls.__deserialize_json_dn_properties(data_node_properties)
properties = cls.__deserialize_json_dn_properties(properties)

if model.storage_type == SQLDataNode.storage_type():
data_node_properties = cls.__deserialize_sql_dn_model_properties(data_node_properties)
properties = cls.__deserialize_sql_dn_model_properties(properties)

if model.storage_type == MongoCollectionDataNode.storage_type():
data_node_properties = cls.__deserialize_mongo_collection_dn_model_properties(data_node_properties)
properties = cls.__deserialize_mongo_collection_dn_model_properties(properties)

if cls._EXPOSED_TYPE_KEY in data_node_properties.keys():
data_node_properties = cls.__deserialize_exposed_type(
data_node_properties, cls._EXPOSED_TYPE_KEY, cls._VALID_STRING_EXPOSED_TYPES
if cls._EXPOSED_TYPE_KEY in properties.keys():
properties = cls.__deserialize_exposed_type(
properties, cls._EXPOSED_TYPE_KEY, cls._VALID_STRING_EXPOSED_TYPES
)
return properties

@classmethod
def _model_to_entity(cls, model: _DataNodeModel) -> DataNode:
data_node_properties = cls._deserialize_properties(model, model.data_node_properties.copy())

validity_period = None
if model.validity_seconds is not None and model.validity_days is not None:
Expand Down
28 changes: 22 additions & 6 deletions tests/core/config/checkers/test_data_node_config_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,12 +513,12 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 2
expected_error_message_1 = (
"`write_query_builder` of DataNodeConfig `new` must be populated with a Callable function."
"`write_query_builder` of DataNodeConfig `new` must be populated with a Callable."
" Current value of property `write_query_builder` is 1."
)
assert expected_error_message_1 in caplog.text
expected_error_message_2 = (
"`append_query_builder` of DataNodeConfig `new` must be populated with a Callable function."
"`append_query_builder` of DataNodeConfig `new` must be populated with a Callable."
" Current value of property `append_query_builder` is 2."
)
assert expected_error_message_2 in caplog.text
Expand All @@ -530,7 +530,7 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 1
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a Callable function. Current value"
"`write_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
" of property `write_fct` is 12.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand All @@ -542,7 +542,7 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 1
expected_error_messages = [
"`read_fct` of DataNodeConfig `new` must be populated with a Callable function. Current value"
"`read_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
" of property `read_fct` is 5.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand All @@ -554,9 +554,9 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 2
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a Callable function. Current value"
"`write_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
" of property `write_fct` is 9.",
"`read_fct` of DataNodeConfig `new` must be populated with a Callable function. Current value"
"`read_fct` of DataNodeConfig `new` must be populated with a Callable. Current value"
" of property `read_fct` is 5.",
]
assert all(message in caplog.text for message in expected_error_messages)
Expand All @@ -581,6 +581,22 @@ def test_check_callable_properties(self, caplog):
Config.check()
assert len(Config._collector.errors) == 0

config._sections[DataNodeConfig.name]["new"].storage_type = "generic"
config._sections[DataNodeConfig.name]["new"].properties = {"write_fct": lambda x: x, "read_fct": lambda y: y}
with pytest.raises(SystemExit):
Config._collector = IssueCollector()
Config.check()
assert len(Config._collector.errors) == 2
expected_error_messages = [
"`write_fct` of DataNodeConfig `new` must be populated with a Callable and not a lambda."
" Current value of property `write_fct` is <function TestDataNodeConfigChecker."
"test_check_callable_properties.<locals>.<lambda>",
"`read_fct` of DataNodeConfig `new` must be populated with a Callable and not a lambda."
" Current value of property `read_fct` is <function TestDataNodeConfigChecker."
"test_check_callable_properties.<locals>.<lambda>",
]
assert all(message in caplog.text for message in expected_error_messages)

Copy link
Member

Choose a reason for hiding this comment

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

BTW, this deserves:

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR I have created for this topic also contains changes required to the doc. https://github.com/orgs/Avaiga/projects/6/views/2?pane=issue&itemId=89479596

def test_check_read_write_fct_args(self, caplog):
config = Config._applied_config
Config._compile_configs()
Expand Down
Loading