Refactoring/#221 fixed mypy warnings #225

ckunki · 2024-11-22T09:38:06Z

Closes #221
Closes #222
Closes #223

Some still need to be clarified

in RegisterPeerForwarder RegisterPeerForwarderBuilderParameter RegisterPeerForwarderFactory

ckunki · 2024-11-25T10:37:47Z

exasol/analytics/udf/communication/peer_communicator/background_listener_thread.py

            predecessor=message.source,
            predecessor_send_socket_factory=predecessor_send_socket_factory,
            successor=message.peer,
            successor_send_socket_factory=successor_send_socket_factory,
            my_connection_info=self._my_connection_info,
        )

+    @property


Property method register_peer_connection checks the private attribute _register_peer_connection and raises an exception when it is None to avoid code duplication.

Questions:

Q1) The names of the private attribute and the property differ on in the prefix _ and potentially could be mixed up. Should we use different names that can easier be distinguished?

Q2) The property is public. Should it be private?

See last push

ckunki · 2024-11-25T11:08:58Z

exasol/analytics/query_handler/graph/stage/sql/execution/query_handler_state.py

+            raise RuntimeError("No current query handler set.")
+        return value
+
+    @property


Property method current_query_handler_context checks the private attribute _current_query_handler_context and raises an exception when it is None to avoid code duplication.

Questions:

Q1) The names of the private attribute and the property differ on in the prefix _ and potentially could be mixed up. Should we use different names that can easier be distinguished?

Q2) The property is public. Should it be private?

If the original is private, the new one needs to be private, too. This means it needs another name. Good question, what the name should be. The same is true for current_stage property. Thinking about a prefix like _checked_*.

👍 I like the proposal _checked_*.

I now used

@property _checked_current_qh_context(self)

attribute self._current_qh_context

See last push

ckunki · 2024-11-25T11:16:50Z

exasol/analytics/schema/column_name_builder.py

@@ -37,6 +39,8 @@ def with_table_like_name(
        return self

    def build(self) -> ColumnName:
+        if self._name is None:
+            raise TypeCheckError("Name must not be None.")


Is it OK to raise a TypeCheckError here (same as typeguard does)?
Raising a different Error such as ValueError would require to update the unit tests.

it is not a typeguard typecheck, so I would create my own TypeCheckError. However, we can do this also in another PR.

ckunki · 2024-11-25T11:18:40Z

exasol/analytics/udf/communication/broadcast_operation.py

@@ -54,8 +55,15 @@ def _receive_from_localhost_leader(self) -> bytes:
        self._check_sequence_number(specific_message_obj=specific_message_obj)
        return frames[1].to_bytes()

+    @property


Property method multi_node_communicator checks the private attribute _multi_node_communicator and raises an exception when it is None to avoid code duplication.

Questions:

Q1) The names of the private attribute and the property differ on in the prefix _ and potentially could be mixed up. Should we use different names that can easier be distinguished?

Q2) The property is public. Should it be private?

See last push

ckunki · 2024-11-25T11:19:33Z

exasol/analytics/udf/communication/gather_operation.py

@@ -94,18 +109,25 @@ def _send_local_leader_message_to_multi_node_leader(self):
            local_position=local_position, value_frame=value_frame
        )

+    @property


Property method multi_node_communicator checks the private attribute _multi_node_communicator and raises an exception when it is None to avoid code duplication.

Questions:

Q1) The names of the private attribute and the property differ on in the prefix _ and potentially could be mixed up. Should we use different names that can easier be distinguished?

Q2) The property is public. Should it be private?

See last push

tkilias · 2024-11-25T11:30:59Z

exasol/analytics/query_handler/context/proxy/bucketfs_location_proxy.py

@@ -1,6 +1,6 @@
 import logging

-import exasol.bucketfs as bfs
+import exasol.bucketfs as bfs  # type: ignore[import-untyped]


I think, we can do this in the pyproject.toml for all imports, but we probably should add py.typed to bucketfs-python

Ah, yes.
I created a ticket for adding file py.typed to bucketfs-python here: exasol/bucketfs-python#173

Thanks to @Nicoretti for helping: We can ignore importing untyped modules in pyproject.toml like this:

[[tool.mypy.overrides]] module = "exasol.analytics.*" disable_error_code = ["import-untyped"]

Additionally type check can be disabled in tests by

[[tool.mypy.overrides]] module = [ "tests.*", "noxfile", ] ignore_errors = true

See last push

exasol/analytics/query_handler/graph/result.py

tkilias · 2024-11-25T11:46:20Z

exasol/analytics/query_handler/python_query_handler_runner.py

@@ -72,8 +73,8 @@ def _handle_continue(self, result: Continue) -> Union[Continue, Finish[ResultTyp
        self._cleanup_query_handler_context()
        self._execute_queries(result.query_list)
        input_query_result = self._run_input_query(result)
-        result = self._state.query_handler.handle_query_result(input_query_result)
-        return result
+        _result = self._state.query_handler.handle_query_result(input_query_result)


Why this rename?

Because result was already a parameter of the method and mypy complained about potentially different types being assigned to the same variable.

tkilias · 2024-11-25T12:00:33Z

exasol/analytics/query_handler/udf/runner/udf.py

+        return self._bucketfs_location
+
+    @property
+    def parameter(self) -> UDFParameter:


these should be private, too

See last push

tkilias · 2024-11-25T12:00:38Z

exasol/analytics/query_handler/udf/runner/udf.py

+        self._parameter = None
+
+    @property
+    def bucketfs_location(self) -> bfs.path.PathLike:


these should be private, too

See last push

tkilias · 2024-11-25T12:03:22Z

exasol/analytics/query_handler/udf/runner/udf.py

@@ -187,10 +211,10 @@ def release_and_create_query_handler_context_if_input_query(
            current_state.top_level_query_handler_context.get_child_query_handler_context()
        )

-    def _get_parameter(self, ctx):
+    def _get_parameter(self, ctx) -> UDFParameter:
        iter_num = ctx[0]


in another PR, we probably should replace the numbers with named constants

tkilias · 2024-11-25T12:06:51Z

exasol/analytics/query_handler/udf/runner/udf.py

    ) -> UDFQueryResult:
        colum_start_ix = 8 if self.parameter.iter_num == 0 else 4
+        if query_columns is None:


questionable, needs more investigation

tkilias · 2024-11-25T12:07:18Z

exasol/analytics/query_handler/python_query_handler_runner.py

@@ -116,24 +117,28 @@ def _release_and_create_query_handler_context_of_input_query(self):
    def _wrap_return_query(
        self, input_query: SelectQueryWithColumnDefinition
    ) -> Tuple[str, str]:
+        if self._state.input_query_query_handler_context is None:


needs investigation

exasol/analytics/udf/communication/messages.py

exasol/analytics/udf/communication/peer_communicator/background_listener_interface.py

exasol/analytics/udf/communication/peer_communicator/background_listener_thread.py

exasol/analytics/udf/communication/socket_factory/fault_injection.py

exasol/analytics/udf/communication/socket_factory/zmq_wrapper.py

exasol/analytics/udf/utils/context_wrapper.py

exasol/analytics/udf/utils/iterators.py

exasol/analytics/utils/errors.py

tomuben

Don't forget to activate the type checker in the CI !!! Otherwise all the work is in vain ;)

Co-authored-by: Thomas Ubensee <[email protected]>

ckunki · 2024-11-27T14:59:21Z

Don't forget to activate the type checker in the CI !!! Otherwise all the work is in vain ;)

I assume the type checker was activated all the time in checks.yml:

run: poetry run nox -s type-check

but mypy was told to ignore all package of the project in pyproject.toml. The latter I updated.

ckunki added 5 commits November 22, 2024 10:34

Fixed mypy warnings

0731aa6

Fixed mypy warnings

40add98

Some still need to be clarified

adde type: ignore to udf_call.py

8cf706c

Fixed type check errors

b2e61fe

updated changelog

c0ef88e

ckunki requested a deployment to slow-tests November 22, 2024 09:38 — with GitHub Actions Waiting

Formatted code

5921a98

ckunki temporarily deployed to slow-tests November 22, 2024 09:56 — with GitHub Actions Inactive

Made register_peer_connection optional

519aceb

in RegisterPeerForwarder RegisterPeerForwarderBuilderParameter RegisterPeerForwarderFactory

ckunki temporarily deployed to slow-tests November 22, 2024 14:15 — with GitHub Actions Inactive

Updated exception strings

1b85f18

ckunki temporarily deployed to slow-tests November 22, 2024 15:22 — with GitHub Actions Inactive

ckunki commented Nov 25, 2024

View reviewed changes

removed invalid entry from pyproject.toml

a2110e4

ckunki requested a deployment to slow-tests November 25, 2024 11:24 — with GitHub Actions Waiting

tkilias reviewed Nov 25, 2024

View reviewed changes

exasol/analytics/query_handler/graph/result.py Show resolved Hide resolved

tkilias reviewed Nov 25, 2024

View reviewed changes

ckunki mentioned this pull request Nov 25, 2024

Replace TypeCheckError by Custom Error #227

Open

3 tasks

ckunki added 2 commits November 25, 2024 15:14

replaced comment to ignore import-untyped by entry in pyproject.toml

a607870

Prefixed properties with "_checked_"

9e3c4da

ckunki requested a deployment to slow-tests November 26, 2024 07:14 — with GitHub Actions Waiting

replace pandas import by expllicit forward references

f493055

ckunki requested a deployment to slow-tests November 26, 2024 08:57 — with GitHub Actions Waiting

Improved type-hints by using ForwardRef

4ecca0c

ckunki requested a deployment to slow-tests November 27, 2024 11:00 — with GitHub Actions Waiting

Fixed ForwardRef

82a4871

ckunki requested a deployment to slow-tests November 27, 2024 11:06 — with GitHub Actions Waiting

Fixed ForwardRef (2)

6ccdf43

ckunki requested a deployment to slow-tests November 27, 2024 12:13 — with GitHub Actions Waiting