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

Backend refactoring #59

Merged
merged 4 commits into from
Jan 20, 2025
Merged

Backend refactoring #59

merged 4 commits into from
Jan 20, 2025

Conversation

jamilraichouni
Copy link
Contributor

Move backend app from data class in explorer.py to a global app in
main.py and launch with uvicorn command.

@jamilraichouni jamilraichouni force-pushed the maintenance/code-revamp branch from da51cb5 to 398ca4d Compare January 13, 2025 15:30
@jamilraichouni jamilraichouni marked this pull request as draft January 13, 2025 15:36
@Wuestengecko
Copy link
Member

Thanks for doing this work! I already have some general comments, just from a first brief look at the diff (so I might have missed something):

  • It seems the middleware updating the last_interaction_time got lost in the process

  • The /metrics endpoint was moved into the app.router (i.e. below $ROUTE_PREFIX), which will probably break monitoring

  • It feels a bit odd to have the app.state setup at the end of the file, and all the rest of the app initialization somewhere further up (not even at the top, right after the imports, just somewhere in the middle between some standalone functions and all the routes). I think we should at least move that first part up to the other globals.

  • In the interest of type safety (read: helping the linters understand what's going on), it might be a good idea to define a dummy dataclass for the app.state, and cast to that instead of using app.state directly.

    If FastAPI doesn't have something clever that we can use built-in, doing it ourselves should be as simple as this:

    @dataclasses.dataclass
    class AppState:
        model: capellambse.MelodyModel
        env: jinja2.Environment
        # etc.
    
    @app.router.get(...)
    def some_route(...):
        state: AppState = app.state

@jamilraichouni jamilraichouni force-pushed the maintenance/code-revamp branch 4 times, most recently from e8db0c1 to df5e8b4 Compare January 14, 2025 13:10
@jamilraichouni jamilraichouni marked this pull request as ready for review January 14, 2025 17:09
@jamilraichouni
Copy link
Contributor Author

Thanks for doing this work! I already have some general comments, just from a first brief look at the diff (so I might have missed something):

  • It seems the middleware updating the last_interaction_time got lost in the process
  • The /metrics endpoint was moved into the app.router (i.e. below $ROUTE_PREFIX), which will probably break monitoring
  • It feels a bit odd to have the app.state setup at the end of the file, and all the rest of the app initialization somewhere further up (not even at the top, right after the imports, just somewhere in the middle between some standalone functions and all the routes). I think we should at least move that first part up to the other globals.
  • In the interest of type safety (read: helping the linters understand what's going on), it might be a good idea to define a dummy dataclass for the app.state, and cast to that instead of using app.state directly.
    If FastAPI doesn't have something clever that we can use built-in, doing it ourselves should be as simple as this:
    @dataclasses.dataclass
    class AppState:
        model: capellambse.MelodyModel
        env: jinja2.Environment
        # etc.
    
    @app.router.get(...)
    def some_route(...):
        state: AppState = app.state

Hi @Wuestengecko , thank you for this review comment!

I considered it and after our discussion I introduced a separate state.py module to handle the application state in a manner that helps mypy to infer the type of state variables.

@Wuestengecko
Copy link
Member

Could you please rebase on top of latest master (at your convenience)? Should be straight forward, as there are no conflicts and no tool changes that should affect formatting of this PR.

@jamilraichouni
Copy link
Contributor Author

Could you please rebase on top of latest master (at your convenience)? Should be straight forward, as there are no conflicts and no tool changes that should affect formatting of this PR.

done

README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
capella_model_explorer/backend/main.py Outdated Show resolved Hide resolved
capella_model_explorer/backend/main.py Outdated Show resolved Hide resolved
capella_model_explorer/backend/main.py Outdated Show resolved Hide resolved
capella_model_explorer/backend/main.py Outdated Show resolved Hide resolved
capella_model_explorer/backend/main.py Outdated Show resolved Hide resolved
@jamilraichouni jamilraichouni force-pushed the maintenance/code-revamp branch 14 times, most recently from 1ab43e8 to d45bde4 Compare January 16, 2025 16:35
@Wuestengecko Wuestengecko changed the title Follow standards with FastAPI app Backend refactoring Jan 17, 2025
jamilraichouni and others added 4 commits January 20, 2025 11:37
Move backend app from a dataclass in `explorer` to a global `app` object
in the new `main` module. Application state is now contained in the new
`state` submodule.

This allows using standard uvicorn commands, and also allows passing
additional uvicorn arguments like `--reload`.
This fixes the "Fetch failed" error in the version display.
The Docker-based pre-commit hooks depent on bind mounts that are
accessible by the host. This commit changes the hooks to use the local
system commands instead of the Docker-based commands.
This ensures that they are actually available for shell substitutions,
even if not passed explicitly.
@Wuestengecko Wuestengecko force-pushed the maintenance/code-revamp branch from d45bde4 to 468b61c Compare January 20, 2025 10:46
@Wuestengecko Wuestengecko merged commit d6fe87e into master Jan 20, 2025
9 checks passed
@Wuestengecko Wuestengecko deleted the maintenance/code-revamp branch January 20, 2025 10:49
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