Move JsonDict import to type-checking path only#19876
Conversation
I ran into the usual case where my `synapse_rust` module was outdated and
needed to rebuild them before running the Complement tests.
However, instead of getting the typical error of `Rust module outdated.
Please rebuild using `poetry install`, I instead got the following:
```
COMPLEMENT_DIR="../complement" ./scripts-dev/complement.sh -run
"TestMSC4429ProfileUpdates"
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/work/code/synapse/synapse/util/__init__.py", line 41, in <module>
from synapse.types import JsonDict
File "/home/work/code/synapse/synapse/types/__init__.py", line 70, in <module>
from synapse.synapse_rust.types import Requester
ModuleNotFoundError: No module named 'synapse.synapse_rust.types'; 'synapse.synapse_rust' is not a package.
```
which is much more confusing for a irregular contributor to
diagnose than the one that tells you to run `poetry install`.
The cause was an edge case: I did not yet have the
`synapse.synapse_rust.types` module, which was added only recently in
#19828. So you could argue
this is a one-time hurdle.
However, the need to import `synapse.synapse_rust.types` solely comes
from needing to import `JsonDict`, which is only used for typechecking!
Hence moving the import under `if typing.TYPE_CHECKING`. With this, I
would have received the more noob-friendly error message instead of one
about a `synapse_rust.types` module not existing.
| @@ -0,0 +1 @@ | |||
| Prevent an obtuse error masking a more user-friendly error when trying to run Synapse with an outdated rust module in certain cases. No newline at end of file | |||
There was a problem hiding this comment.
Hmm, this won't help for future cases though. The root cause is that we started importing stuff from Rust before we checked if the Rust was up to date. This is because check_rust_lib_up_to_date lives in synapse.util.rust, but we import Rust stuff in synapse.util.
I think we should move check_rust_lib_up_to_date into a top-level module and make sure that it is imported/called before anything else is imported (other than maybe the Python version check). I'm tempted to have something like synapse.startup_checks that checks the appropriate things on import with no/minimal imports from other synapse modules.
Thoughts?
There was a problem hiding this comment.
(FTR you can stop an import line being resorted by adding a # isort: skip comment after it)
There was a problem hiding this comment.
That does sound more correct, and future-proof. I'll give it a shot!
I ran into the usual case where my
synapse_rustmodule was outdated and needed to rebuild them before running the Complement tests.However, instead of getting the typical error of
Rust module outdated. Please rebuild usingpoetry install`, I instead got the following:which is much more confusing for a irregular contributor to diagnose than the one that tells you to run
poetry install.The cause was an edge case: I did not yet have the
synapse.synapse_rust.typesmodule, which was added only recently in #19828. So you could argue this is a one-time hurdle.However, the need to import
synapse.synapse_rust.typessolely comes from needing to importJsonDict, which is only used for typechecking!Hence moving the import under
if typing.TYPE_CHECKING. With this, I would have received the more noob-friendly error message instead of one about asynapse_rust.typesmodule not existing.Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.