-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix config reload state imports (#7028) #7035
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Prevent `reload_config()` from raising a duplicate-state error for state modules imported by `rxconfig.py`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -803,10 +803,12 @@ def _set_persistent(self, **kwargs): | |
| self._replace_defaults(**kwargs) | ||
|
|
||
|
|
||
| # Project-local modules first imported while loading rxconfig.py; evicted | ||
| # before the next load so projects don't reuse each other's dependencies. | ||
| # Only mutated under _load_config_lock. | ||
| # Project-local modules first imported while loading rxconfig.py and the root | ||
| # that supplied them. They are evicted before loading a different project so | ||
| # projects don't reuse each other's dependencies. Only mutated under | ||
| # _load_config_lock. | ||
| _config_module_deps: set[str] = set() | ||
| _config_module_deps_root: Path | None = None | ||
|
|
||
|
|
||
| class _ImportRecorder: | ||
|
|
@@ -894,7 +896,9 @@ def get_state_auto_setters() -> bool: | |
| return False | ||
|
|
||
|
|
||
| def _get_config(project_root: Path | None = None) -> Config: | ||
| def _get_config( | ||
| project_root: Path | None = None, *, reload_dependencies: bool = True | ||
| ) -> Config: | ||
| """Import rxconfig.py fresh from the project root and return its config. | ||
|
|
||
| The project root is prepended to sys.path for the duration of the import so | ||
|
|
@@ -907,10 +911,15 @@ def _get_config(project_root: Path | None = None) -> Config: | |
| current working directory, resolved once up front so an rxconfig.py | ||
| that changes the cwd cannot move the root that the sys.path entry | ||
| and the dependency classification below are based on. | ||
| reload_dependencies: Whether to reload project-local modules imported by | ||
| rxconfig.py. A config reload in an existing RegistrationContext | ||
| keeps them so state classes are not redefined. | ||
|
|
||
| Returns: | ||
| The app config. | ||
| """ | ||
| global _config_module_deps_root | ||
|
|
||
| project_root = (project_root or Path.cwd()).resolve() | ||
| with _load_config_lock: | ||
| # A fresh str object, so the exact inserted entry can be removed by | ||
|
|
@@ -919,16 +928,17 @@ def _get_config(project_root: Path | None = None) -> Config: | |
| cwd = str(project_root) | ||
| sys.path.insert(0, cwd) | ||
| try: | ||
| # Never cache rxconfig or its project-local dependencies — each load | ||
| # goes to disk so different RegistrationContexts hold independent | ||
| # Config instances resolved against the current project. Evict | ||
| # before probing: find_spec answers from sys.modules, so modules | ||
| # left behind by another project directory would fake the existence | ||
| # check below. | ||
| # Always reload rxconfig, but retain its dependencies when reloading | ||
| # the same project. Re-importing a helper that defines State would | ||
| # redefine the class in the active RegistrationContext. Before | ||
| # switching projects, evict dependencies so find_spec and imports | ||
| # cannot reuse modules from the prior root. | ||
| sys.modules.pop(constants.Config.MODULE, None) | ||
| for dep in _config_module_deps: | ||
| sys.modules.pop(dep, None) | ||
| _config_module_deps.clear() | ||
| if reload_dependencies or _config_module_deps_root != project_root: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Either drop |
||
| for dep in _config_module_deps: | ||
| sys.modules.pop(dep, None) | ||
| _config_module_deps.clear() | ||
| _config_module_deps_root = project_root | ||
| # only import the module if it exists. If a module spec exists then | ||
| # the module exists. | ||
| if not find_spec(constants.Config.MODULE): | ||
|
|
@@ -1021,6 +1031,6 @@ def reload_config() -> Config: | |
| The freshly loaded app config. | ||
| """ | ||
| ctx = RegistrationContext.ensure_context() | ||
| config = _get_config() | ||
| config = _get_config(reload_dependencies=ctx._config is None) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When an older Prompt for AI agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Repro with the with RegistrationContext() as ctx:
c.get_config()
forked = ctx.fork()
tok = RegistrationContext.set(forked)
c.reload_config()
# StateValueError: The substate class 'appmod____my_state' has been defined multiple times.This is the path |
||
| ctx._set_config(config) | ||
| return config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1029,20 +1029,118 @@ def test_get_config_accepts_explicit_project_root( | |
| assert reflex_base.config._get_config(project).app_name == "explicit" | ||
|
|
||
|
|
||
| def test_reload_config_does_not_redefine_project_state( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None | ||
| ): | ||
| """Reloading config does not re-import project modules that define state. | ||
|
|
||
| Args: | ||
| tmp_path: The pytest tmp_path fixture. | ||
| monkeypatch: The pytest monkeypatch fixture. | ||
| clean_config_modules: Cleanup for modules left behind by the load. | ||
| """ | ||
| from reflex_base.registry import RegistrationContext | ||
|
|
||
| (tmp_path / "config_reload_state_module.py").write_text( | ||
| "import reflex as rx\n\nclass MyState(rx.State):\n value: str = ''\n" | ||
| ) | ||
| (tmp_path / "rxconfig.py").write_text( | ||
| "import config_reload_state_module\nimport reflex as rx\n\n" | ||
| "config = rx.Config(app_name='state_reload')\n" | ||
| ) | ||
| monkeypatch.chdir(tmp_path) | ||
|
|
||
| with RegistrationContext(): | ||
| assert reflex_base.config.get_config().app_name == "state_reload" | ||
| assert reflex_base.config.reload_config().app_name == "state_reload" | ||
|
|
||
|
|
||
| def test_get_config_reloads_project_state_for_each_context( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None | ||
| ): | ||
| """Initial config loads register project state in each context. | ||
|
|
||
| Args: | ||
| tmp_path: The pytest tmp_path fixture. | ||
| monkeypatch: The pytest monkeypatch fixture. | ||
| clean_config_modules: Cleanup for modules left behind by the load. | ||
| """ | ||
| from reflex_base.registry import RegistrationContext | ||
|
|
||
| (tmp_path / "config_reload_state_module.py").write_text( | ||
| "import reflex as rx\n\nclass MyState(rx.State):\n value: str = ''\n" | ||
| ) | ||
| (tmp_path / "rxconfig.py").write_text( | ||
| "import config_reload_state_module\nimport reflex as rx\n\n" | ||
| "config = rx.Config(app_name='state_reload')\n" | ||
| ) | ||
| monkeypatch.chdir(tmp_path) | ||
|
|
||
| with RegistrationContext() as first_context: | ||
| reflex_base.config.get_config() | ||
| first_state = next( | ||
| state | ||
| for state in first_context.base_states.values() | ||
| if state.__module__ == "config_reload_state_module" | ||
| ) | ||
|
|
||
| with RegistrationContext() as second_context: | ||
| reflex_base.config.get_config() | ||
| second_state = next( | ||
| state | ||
| for state in second_context.base_states.values() | ||
| if state.__module__ == "config_reload_state_module" | ||
| ) | ||
|
|
||
| assert second_state is not first_state | ||
|
|
||
|
|
||
| def test_get_config_evicts_dependencies_from_another_project( | ||
| tmp_path: Path, clean_config_modules: None | ||
| ): | ||
| """Loading another project does not reuse a same-named local dependency. | ||
|
|
||
| Args: | ||
| tmp_path: The pytest tmp_path fixture. | ||
| clean_config_modules: Cleanup for modules left behind by the load. | ||
| """ | ||
| first_project = tmp_path / "first" | ||
| second_project = tmp_path / "second" | ||
| for project, app_name in ((first_project, "first"), (second_project, "second")): | ||
| project.mkdir() | ||
| (project / "config_reload_dependency.py").write_text( | ||
| f"APP_NAME = {app_name!r}\n" | ||
| ) | ||
| (project / "rxconfig.py").write_text( | ||
| "import config_reload_dependency\nimport reflex as rx\n\n" | ||
| "config = rx.Config(app_name=config_reload_dependency.APP_NAME)\n" | ||
| ) | ||
|
|
||
| assert reflex_base.config._get_config(first_project).app_name == "first" | ||
| assert reflex_base.config._get_config(second_project).app_name == "second" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both calls use |
||
|
|
||
|
|
||
| @pytest.fixture | ||
| def clean_config_modules() -> Generator[None, None, None]: | ||
| """Drop the modules and dep records a real rxconfig load leaves behind. | ||
|
|
||
| Yields: | ||
| None, once the module table is clean. | ||
| """ | ||
| names = ("rxconfig", "side_module", "chdir_dep_module") | ||
| names = ( | ||
| "rxconfig", | ||
| "side_module", | ||
| "chdir_dep_module", | ||
| "config_reload_state_module", | ||
| "config_reload_dependency", | ||
| ) | ||
| try: | ||
| yield | ||
| finally: | ||
| for name in names: | ||
| sys.modules.pop(name, None) | ||
| reflex_base.config._config_module_deps.clear() | ||
| reflex_base.config._config_module_deps_root = None | ||
|
|
||
|
|
||
| # Reruns: taking the prepended entry back out is itself a sys.path shrink, so | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too long comments, explaining what is already can be seen in code. happens in many places.
we might want to clean up these.