Refactor: pems_streamlit package
#184
Conversation
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
3 times, most recently
from
39e2055 to
cbc737f
Compare
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
thekaveman
force-pushed
the
refactor/pems-web
branch
from
bac7c2f to
136035c
Compare
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
from
cbc737f to
1c0dfec
Compare
thekaveman
force-pushed
the
refactor/pems-web
branch
from
136035c to
2b87130
Compare
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
from
1c0dfec to
e259419
Compare
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
from
e259419 to
3ed130e
Compare
There was a problem hiding this comment.
We can probably leave this file blank since we now have a real Streamlit application in the repo. Also, with the new pems_streamlit package, the code in this file is getting ran whenever the package is imported the first time and we probably don't want that to happen.
There was a problem hiding this comment.
Since we don't need a default app page anymore, we can remove _default_app_page(), creating the default app page, and inserting the default app page in this file.
update the build into a multi-stage: - first build the pems_streamlit wheel - then install it with other necessary tooling similar to how the pems_web Docker build works
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
from
774bb4e to
8a3ce21
Compare
|
@lalver1
|
There was a problem hiding this comment.
This looks great @thekaveman! The editable installs now work, so when I run the debugger I can see the changes I make to the templates in the source.
Just one thing, we removed the default Streamlit page but it looks like st.navigation doesn't like this, it seems to expect at least one page tagged as a default page:
Page not found
The page that you have requested does not seem to exist. Running the app's main page.
But, the comment below fixes it. I tried the change locally and I don't get the error shown above.
| @@ -51,8 +47,6 @@ def _make_app_page(app_path: Path) -> StreamlitPage: | |||
|
|
|||
|
|
|||
| def discover_apps() -> list[StreamlitPage]: | |||
| default_app_page = _default_app_page() | |||
There was a problem hiding this comment.
Looks like we have to add back in _default_app_page(), creating the default app page, and inserting the default app page to have at least one page tagged as default. But we still leave the __init__.py blank as we have it right now.
There was a problem hiding this comment.
Thanks! I should have checked. Fixed and rebased.
pip installing the root-level pyproject.toml with package dependencies didn't install them in editable mode, meaning it was difficult to work with the code in the devcontainer instead explicitly pip install each package as editable in the Docker build process
thekaveman
force-pushed
the
refactor/pems-streamlit
branch
from
7e174b4 to
9be9a63
Compare
Closes #178
What this PR does
Follows the same general approach and outline from #181
streamlit_appbecomespems_streamlitRename the project directory following the new naming convention. This is now a true Python package with its own
pyproject.tomllisting dependencies and other metadata. The code is reorganized into thesrcstructure similar topems_webin #181.Docker build process
Updates the Docker build into two stages, where the first builds a wheel for
pems_streamlitand the second configures the runtime environment and installs the wheel.Devcontainer
Update the package installation to ensure each is installed in editable mode. Instead of using the root level
pyproject.toml, install them explicitly in the devcontainer Docker build.Tests
Ensures coverage is reported for the new
pems_streamlitpackage.