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

[ENG-4134]Allow specifying custom app module in rxconfig #4556

Merged
merged 16 commits into from
Jan 20, 2025

Conversation

ElijahAhianyo
Copy link
Contributor

@ElijahAhianyo ElijahAhianyo commented Dec 18, 2024

import sys
import reflex as rx
from pathlib import Path


sys.path.insert(0, str(Path("/path/to/app/foo/")))

config = rx.Config(
    app_name="my_project",
    app_module_path="foo.foo"
)

Assuming you have a reflex app foo with App instance living in foo/foo/foo.py

Example app: reflex-dev/reflex-examples#291

Copy link

linear bot commented Dec 18, 2024

reflex/config.py Outdated
@property
def module(self) -> str:
"""Get the module name of the app.

Returns:
The module name.
"""
if self.app_module:
return f"{Path(self.app_module.__file__).parent.name}.{Path(self.app_module.__file__).stem}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wondering if this is what we should be doing. Or should we return the absolute path here? Im trying to think of where doing it like this could pose a real problem

@ElijahAhianyo ElijahAhianyo force-pushed the elijah/app-in-rxconfig branch from 2bbdb4c to c9d967e Compare January 13, 2025 10:49
@ElijahAhianyo ElijahAhianyo changed the title [WIP][ENG-4134]Allow specifying custom app module in rxconfig [ENG-4134]Allow specifying custom app module in rxconfig Jan 13, 2025
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review January 13, 2025 16:49
@ElijahAhianyo
Copy link
Contributor Author

integration tests need reflex-dev/reflex-examples#291 to pass

reflex/config.py Outdated
"""
module_name = Path(path).stem
module_path = Path(path).resolve()
sys.path.insert(0, str(module_path.parent.parent))
Copy link
Collaborator

@masenf masenf Jan 15, 2025

Choose a reason for hiding this comment

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

i don't like the assumption that the pythonpath entry is module_path.parent.parent.

i think a better approach would make the caller responsible for setting their sys.path / PYTHONPATH, and then providing an importable name.

That would save us this complicated import machinery that makes assumptions that would be hard to undo later, we could just use __import__, like we have been.

Comment on lines 251 to 252
if app_module_path := config.app_module_path:
reload_dirs.append(str(Path(app_module_path).resolve().parent.parent))
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think for this one, we should go up the parent directories until we don't find an __init__.py, this should be the root of the reload dir for an out-of-tree app module.

@masenf
Copy link
Collaborator

masenf commented Jan 15, 2025

updated by comment from before. i realize we can't actually instantiate an rx.App from inside rxconfig.py because app initialization depends on getting the config

@masenf
Copy link
Collaborator

masenf commented Jan 15, 2025

@ElijahAhianyo i opened a PR into your branch #4644 for your review

* reload_dirs: search up from app_module for last directory containing __init__

* Change custom app_module to use an import string

* preserve sys.path entries added while loading rxconfig.py
@ElijahAhianyo
Copy link
Contributor Author

@masenf I think we need reflex-dev/reflex-examples#291 merged to pass the CI and then it's ready to go

@masenf masenf merged commit 268effe into main Jan 20, 2025
41 checks passed
@masenf masenf deleted the elijah/app-in-rxconfig branch January 20, 2025 18:12
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.

2 participants