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

feat: Integrate FastHTML replacing FastAPI and React #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamilraichouni
Copy link
Contributor

@jamilraichouni jamilraichouni commented Jan 27, 2025

This introduces several major changes by

  • replacing the FastAPI backend and React frontend with FastHTML and
  • replacing the Makefile with a Python CLI tool that manages the project.

The project can be managed including

  • building the Docker image,
  • running the app in a container,
  • running the app locally with optional full hot code reloading, and
  • building the Tailwind CSS stylesheet.

New is also a hot template reloading, which is enabled per default.

In development mode, it also runs with hot code reloading.

The FastHTML integration involves reusing existing code where possible, but also includes significant rewrites that affect the application's behavior. This update aims to enhance performance and streamline the development process.

Open issues/ topics

  1. fix: Make env var ROUTE_PREFIX work for static files
  2. chore: Build stylesheet always via Tailwind CLI.
  3. fix: CI job docker must set tool versions via three --build-arg specs
  4. fix: Improve CSS for rendered templates output
  5. fix: Make CME_ROUTE_PREFIX env var work
  6. fix: Move state (current template, current model element, breadcrumbs) to client as discussed with @Wuestengecko
  7. feat: Implement "Click to enlarge" on hover for SVGs which opens a modal dialog with pan/ zoom/ print/ close functionality for a diagram
  8. chore: Consider (reuse) plotly license
  9. fix: Make print in diagramViewer.js work (scaling must be fixed for diagrams with a height which is greater than the width)
  10. feat: Show traceback when running into RuntimeError when rendering template "Generic object description" #82
  11. chore: Cache headers for statics
  12. docs: Update README.md/ CONTRIBUTING.md
  13. Add some basic tests (and restore the hint in CONTRIBUTING), see docs: Restore hint to run units tests #90
  14. Amend commit message subject so that it is marked (!) that this PR introduces breaking changes.

Integration in Capella Collaboration Manager

  1. Rename the env vars in CCM tool cfg MODEL_ENTRYPOINT --> CME_MODEL and ROUTE_PREFIX --> CME_ROUTE_PREFIX Align this with main contributor @MoritzWeber0 of https://github.com/DSD-DBS/capella-collab-manager.git
  2. Ensure, that CCM sets env var CME_LIVE_MODE=0 in the tool config

@jamilraichouni jamilraichouni force-pushed the fasthtml branch 5 times, most recently from b7cde84 to e0c22bf Compare January 27, 2025 23:22
Copy link
Member

@Wuestengecko Wuestengecko left a comment

Choose a reason for hiding this comment

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

Here are some initial comments. As the PR is still WIP anyway, I've mostly only looked at the greater ideas, and not too much into the details yet. (Except for the comment about OOB swaps :p )

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to store all these icons as individual *.svg files and serve them somewhere below /static. This allows the browser to cache the files and optimize rendering, since it knows that they must look the same (in the absence of CSS shenanigans, that is). It also makes the licensing situation a bit clearer, as there can be individual license notices attached to each file. Plus, it slightly improves server performance as well, as there's no longer the need to construct several intermediate objects over and over just to show a static image.

Copy link
Contributor Author

@jamilraichouni jamilraichouni Jan 28, 2025

Choose a reason for hiding this comment

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

I thought about having .svg files instead of py components for SVGs.
Do not want to do that because we depend on Tailwind CSS utility classes which will be listed in the class (respectively FastHTML's cls) attribute of the <svg> tags sent to the client instead of attrs of the <svg> tag itself.

With external .svg file we would have final values for colors. A solution with string replacements after the reading of SVG files is sth. I do not really want to implement.

We want to be able to configure colors

<svg stroke="black">  <!-- light theme -->
<svg stroke="white">  <!-- dark theme -->

Example pseudo-code of current implementation:

constants.py

STROKE_ACCENT: t.Final[dict[str, str]] = {
    "dark": "white",
    "light": "black"
}

icons.py

from fasthtml import svg  

import constants as c

THEME: str = "dark"

def icon() -> t.Any:
    return fh.Svg(
        svg.Path(...),
        cls=f"stroke-{c.STROKE_ACCENT[THEME]}""
    )

I am open for discussion. It has for sure benefits when dealing with .svg files which can easily be edited/ downloaded etc..

capella_model_explorer/main.py Outdated Show resolved Hide resolved
capella_model_explorer/components.py Outdated Show resolved Hide resolved
.github/workflows/build-test-publish.yml Outdated Show resolved Hide resolved
capella_model_explorer/components.py Outdated Show resolved Hide resolved
Comment on lines +146 to +167
ft.Div(model_element["uuid"], cls="text-xs text-sky-700")
if state.show_uuids
else None,
Copy link
Member

Choose a reason for hiding this comment

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

In the future, we'll probably want to make this a client- (or session-) specific setting that's configurable via some settings UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #102

capella_model_explorer/main.py Outdated Show resolved Hide resolved
capella_model_explorer/main.py Outdated Show resolved Hide resolved
@jamilraichouni jamilraichouni force-pushed the fasthtml branch 24 times, most recently from 8e79d21 to 2391551 Compare January 30, 2025 13:23
@jamilraichouni jamilraichouni force-pushed the fasthtml branch 3 times, most recently from 6d7bbb9 to 0134fe1 Compare February 7, 2025 08:57
@Wuestengecko
Copy link
Member

New is also an always enabled hot template reloading.

Please add a way to disable that.

uvicorn's live reloading feature injects a websocket connection that reloads the page automatically. Problem with that is that it also tries to reload the page when that websocket connection breaks, for example due to the server going down. This reload then of course fails, because the server is down now, and just shows an error page. So the effect is that you can't just keep an old version of a page opened in the background. I want to be able to do just that, for example to compare against a newer template version, or just to look at the current DOM and make template changes without constantly being thrown around in the page inspector due to the reloads.

Also, the feature should be disabled when deploying the docker image to e.g. the Collab Manager, where the templates are immutable anyway.

@jamilraichouni jamilraichouni force-pushed the fasthtml branch 6 times, most recently from 6b86c3c to cdd0938 Compare February 7, 2025 12:37
@jamilraichouni jamilraichouni marked this pull request as ready for review February 7, 2025 12:49
CONTRIBUTING.md Outdated Show resolved Hide resolved
@jamilraichouni jamilraichouni force-pushed the fasthtml branch 3 times, most recently from f46b53b to 8c8f1a5 Compare February 9, 2025 07:11
Copy link
Member

@Wuestengecko Wuestengecko left a comment

Choose a reason for hiding this comment

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

Haven't looked at much of the actual code yet. More important stuff came up, so I don't know when I'll be able to. So here you go.

.prettierignore Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
pyproject.toml Outdated
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
"Programming Language :: Python :: 3.13",
"Development Status :: 1 - Planning",
Copy link
Member

Choose a reason for hiding this comment

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

Why did you also change the indentation width of this entire file? We're using 2 spaces in Jinja as well, so it's not like everything else is 4 spaces anyway. This just seems unnecessary to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mysterious effect. fixed that. will appear with next push.

pyproject.toml Outdated Show resolved Hide resolved
static/README.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a README for /static in a web app 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idea was to document how the .ico can be made. forgot to push the locally got ignored dev subfolder.
after we checked that chrome understands to display .svg favicons directl too, i remove this anyway.

Comment on lines 87 to 91
subprocess.run(
cmd,
check=True,
capture_output=False,
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subprocess.run(
cmd,
check=True,
capture_output=False,
)
subprocess.check_call(cmd)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx. done.

)


def run_local():
Copy link
Member

Choose a reason for hiding this comment

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

cme run local needs to build the CSS if it doesn't exist already (for example after a fresh clone).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice one. thx!

]
)
cmd.append("capella_model_explorer.main:app")
print(" ".join(cmd))
Copy link
Member

Choose a reason for hiding this comment

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

Left-over debug print.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, do this after all of the cmd runs. want to leave that including the non-dev case run local.
may help to get this line from anybody, if sth went wrong...

Comment on lines 137 to 138
thread = threading.Thread(target=build_css, kwargs={"watch": True})
thread.start()
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a thread. We can simply do the launch preparation serially, then start the two processes with Popen(), and await both of them using:

with uvicorn, tailwindcli:
    time.sleep(float("inf"))

Comment on lines 96 to 97
if not pathlib.Path(c.TEMPLATES_DIR).is_dir():
raise SystemExit(f"Templates directory '{c.TEMPLATES_DIR}' not found.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not pathlib.Path(c.TEMPLATES_DIR).is_dir():
raise SystemExit(f"Templates directory '{c.TEMPLATES_DIR}' not found.")
if not pathlib.Path(c.TEMPLATES_DIR).is_dir():
raise SystemExit(f"Templates directory not found: {c.TEMPLATES_DIR}")

@jamilraichouni jamilraichouni force-pushed the fasthtml branch 4 times, most recently from 1e69029 to 2173540 Compare February 10, 2025 11:26
@jamilraichouni
Copy link
Contributor Author

New is also an always enabled hot template reloading.

Please add a way to disable that.

uvicorn's live reloading feature injects a websocket connection that reloads the page automatically. Problem with that is that it also tries to reload the page when that websocket connection breaks, for example due to the server going down. This reload then of course fails, because the server is down now, and just shows an error page. So the effect is that you can't just keep an old version of a page opened in the background. I want to be able to do just that, for example to compare against a newer template version, or just to look at the current DOM and make template changes without constantly being thrown around in the page inspector due to the reloads.

Also, the feature should be disabled when deploying the docker image to e.g. the Collab Manager, where the templates are immutable anyway.

One can set CME_LIVE_MODE=0. My PR comment was misleading. See also:

cme run local -h

# }}}

# install Nodep kgs and build the CSS {{{
RUN npm clean-install && uv run python3 -m capella_model_explorer build css
Copy link
Member

Choose a reason for hiding this comment

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

We don't even need a multi-stage build, we can just remove the node_modules right here. They're not needed after this step anymore, as far as I can tell.

Suggested change
RUN npm clean-install && uv run python3 -m capella_model_explorer build css
RUN npm clean-install && uv run python3 -m capella_model_explorer build css && rm -rf node_modules

@jamilraichouni jamilraichouni force-pushed the fasthtml branch 2 times, most recently from 5b3b2ea to 2e36557 Compare February 10, 2025 13:45
This introduces a major change by replacing the FastAPI backend and
React frontend with FastHTML. The integration involves reusing existing
code where possible, but also includes significant rewrites that affect
the application's behavior. This update aims to enhance performance and
streamline the development process.

This commit also introduces project management tool which replaces the
Makefile with a Python CLI tool. The tool manages the project, including
building the Docker image, running the app in a container, and running
the app locally. The tool also builds the Tailwind CSS stylesheet.

The app always runs with hot template reloading enabled.
In development mode, it also runs with hot code reloading.

After cloning the project, one can get further information by running:

```bash
cme -h
cme run -h
cme build -h
```

Co-authored-by: Martin Lehmann <[email protected]>
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