-
Notifications
You must be signed in to change notification settings - Fork 415
feat/3198-add-workspace-info-and-profile-selection #3295
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
feat/3198-add-workspace-info-and-profile-selection #3295
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 1559ead | Commit Preview URL Branch Preview URL |
Nov 13 2025, 07:37 PM |
3ebbc28 to
1ac9e43
Compare
sh-rp
left a comment
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.
The implementation in the dashboard app is very good. I have some small notes there and this one: When you have selected a pipeline and switch to a profile where this pipeline has not been run, the app crashes. You should get a helpful error message there.
For the tests we can do the following:
- Let's convert the previously existing tests in such a way, that the dashoard server gets launched and killed from the test code too, then we can get rid of the extra makefile command for this.
- For the new tests:
- Create a new example workspace in
tests/workspaces/caseswhich has two pipelines. You should be able to put all the needed code for the workspace in there including different settings for the profiles. You can even change the pipeline name via a config setting so we can more easy test later. - Use with
isolated_workspaceto set up a workspace in the e2e tests and start the dashboad in the cwd of this workspace. You should be able to get the basefolder of this isolated workspace and set it for the dashboard command. When you run pipelines with various profiles in this isolated workspace, you should see the dashboard populating
- Create a new example workspace in
|
|
||
| def build_workspace_label() -> Any: | ||
| """Show workspace name, or '_' if not set/available.""" | ||
| run_context = dlt.current.run_context() |
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.
Here you should check wether it is a WorkspaceRunContext, if it is not, then this whole workspace section should not appear. See comment above.
| return mo.vstack(_result) | ||
|
|
||
|
|
||
| def build_run_context_inline_label() -> Any: |
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.
This lable / dropdown should not appear at all in a non-workspace dashboard. If a user opens the dashboad in a folder where there is no workspace, it should just look the way it did before.
|
|
||
| def build_labeled_inline(label_text: str, content: Any) -> Any: | ||
| """Inline label + content, matching selector header design.""" | ||
| return mo.hstack([mo.md(f"<small>{label_text}:</small>"), content], align="center").style( |
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.
I think you can set the gap directly on the hstack, no need to call style:
https://docs.marimo.io/api/layouts/stacks/
| ) | ||
|
|
||
|
|
||
| def build_tabs_spacer(num_tabs: int = 2) -> Any: |
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.
Can this not also be solved by a gap on the stack that these items are where you use this?
https://docs.marimo.io/api/layouts/stacks/. Adding tabs is a bit hacky.
| """Discover profiles and return a single-select multiselect, similar to pipelines.""" | ||
| run_context = dlt.current.run_context() | ||
|
|
||
| def _profiles_from_run_context(): |
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.
this does not need to be an inner function, you can just write all the code without wrapping it, no?
|
|
||
|
|
||
| @app.cell(hide_code=True) | ||
| def utils_discover_pipelines( |
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.
I think you can revert all changes in this function, it should still work.
- Added a dropdown for profile selection in the dashboard interface. - Updated the layout to display profile and workspace information inline with pipeline selection. - Introduced a new utility function to discover profiles and return a dropdown for selection. - Added a new Makefile target for running profile-specific end-to-end tests. - Created a new test file for end-to-end testing of profile functionality in the dashboard.
- Modified Makefile to exclude 'test_profiles_e2e' from e2e tests for both headless and headed runs. - Updated GitHub Actions workflow to reflect the same exclusion for dashboard e2e tests, ensuring consistency across local and CI environments.
- Introduced inline controls for profile and workspace information in the dashboard. - Updated layout to improve the organization of elements, ensuring a cleaner interface. - Simplified utility functions for profile discovery and selection. - Adjusted labels for dropdowns to provide clearer context for users.
- Streamlined the profile discovery process by consolidating checks for profile context. - Enhanced dropdown initialization for profile selection, ensuring it reflects the current state and available options. - Improved the handling of query parameters and CLI arguments for profile switching, maintaining user context more effectively.
…e handling - Reordered parameters in the home function to enhance clarity. - Updated the utils_discover_pipelines function to include profile selection logic, ensuring the correct profile is active during pipeline discovery. - Modified utils_discover_profiles to streamline profile selection and remove redundant profile switching logic. - Enhanced the utils_cli_args_and_query_vars_config function to better manage query parameters and CLI arguments related to profiles.
- Updated Makefile to simplify e2e test commands by removing exclusions for profile tests. - Enhanced GitHub Actions workflow to align with the updated test commands. - Introduced new fixtures in `conftest.py` for better management of dashboard server and pipeline setups. - Added a new test for workspace profiles to validate profile-specific behavior in the dashboard. - Removed outdated `test_profiles_e2e.py` file to streamline the test suite.
3fe268b to
7a0de28
Compare
5665454 to
4e0ea46
Compare
…ompatibility - Introduced a new utility function `_normpath` in `conftest.py` to ensure paths are normalized to Unix style and lowercase on Windows.
7665236 to
aa9f8df
Compare
rudolfix
left a comment
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.
LGTM!
I changed a few thing to make it work on windows. All the problems were coming from how marimo was launched: a child process was spawned that created another child process with actual server. that was unkillable in windows and was leaving running processes and locked files
13c7df2 to
1946293
Compare
rudolfix
left a comment
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.
LGTM again!
Description
Related Issues
3198
How to Test
Run profile e2e tests:
make dashboard-test-profilesManually:
Additional Context