Conversation
Refactor routes. Add README.
Fingel
left a comment
Fingel
left a comment
There was a problem hiding this comment.
I spent some time trying to get this running locally, but was ultimately unsuccessful.
I'm not clear on how the database is managed. There are some manual commands in the file database_postgresql_notes but if that is the de-facto method of getting a database set up, shouldn't it be a .sql file?
There seemed to be some syntax errors in there anyway, ($$to add enum type -> alter type bandpass add value 'other';) so even after copying the commands to a .sql file I was unable to use them to set up the database properly.
Without the database, it seems the tests will not run. Additionally, the tests appear to make HTTP requests to 2 different services. I managed to figure that one of them running on port 8000 was the fastapi service itself. But I don't know what the requests going to port 5000 are supposed to test.
All in all, I think the README needs some help.
| Flask-Mail==0.10.0 | ||
| Flask-SQLAlchemy==2.5.1 | ||
| Flask-WTF | ||
| flask-caching |
Sorry about that - I've added a bit of signposting from the main readme to the fastapi readme. With the project in flux with the intention to retire flask in favour of fastapi / svelte I wasn't concentrating so much on the top level README. In summary, the most straightforward way to get the whole thing up and running - fastapi, database, even the flask app, is to use skaffold - just as Hope that is a bit clearer now. |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the legacy Flask backend with a new async FastAPI application, adds Pydantic-based settings and JWT authentication, and integrates full Kubernetes/Helm/Skaffold deployment plus comprehensive endpoint tests.
- Introduce a modern FastAPI service in
server/with SQLAlchemy models, Pydantic schemas, and JWT auth - Provide Pydantic settings in
server/config.pyand Dockerfile for containerization - Add Helm charts (
gwtm-helm/) and Skaffold config for local and production deployments
Reviewed Changes
Copilot reviewed 147 out of 147 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/core/enums/bandpass.py | New bandpass enum values |
| server/core/enums/init.py | Import list for core enums (duplicate import present) |
| server/config.py | Pydantic Settings class for environment variables |
| server/auth/auth.py | JWT token creation, decoding, and user validation |
| server/auth/init.py | Auth package init |
| server/README.md | FastAPI backend usage and testing instructions |
| server/Dockerfile | Container build for FastAPI service |
| gwtm-helm/values.yaml | Helm values for FastAPI deployment |
| gwtm-helm/values-dev.yaml | Development overrides for Helm |
| gwtm-helm/templates/ingress.yaml | Ingress paths for /api/v1, docs, health |
| gwtm-helm/templates/fastapi/service.yaml | Kubernetes Service definition for FastAPI |
| gwtm-helm/templates/fastapi/deployment.yaml | Kubernetes Deployment for FastAPI with init container |
| gwtm-helm/templates/fastapi/configmap.yaml | ConfigMap for FastAPI environment variables |
| gwtm-helm/templates/backend/create-database-tables.yaml | Database setup Job |
| gwtm-helm/start-dev.sh | Script to start Skaffold dev environment |
| gwtm-helm/skaffold.yaml | Skaffold configuration for both Flask and FastAPI |
| gwtm-helm/restore-db | Script for restoring database dumps |
| gwtm-helm/fastapi-README.md | Documentation for FastAPI Helm templates |
| README.md | Root README with FastAPI quick-start |
| .dockerignore | Updated ignore rules for Docker build |
Comments suppressed due to low confidence (2)
server/core/enums/bandpass.py:3
- Class names should use PascalCase. Rename
bandpasstoBandpassto follow Python class naming conventions.
class bandpass(IntEnum):
gwtm-helm/templates/fastapi/configmap.yaml:13
CORS_ORIGINSis expected to be a list by the Pydantic settings. Providing a single string may cause parsing errors. Consider formatting it as a JSON array or comma-separated list that your app can split.
CORS_ORIGINS: "{{ .Values.ingress.host }}"
server/core/enums/__init__.py
Outdated
| from .pointing_status import pointing_status | ||
| from .frequency_units import frequency_units | ||
| from .depth_unit import depth_unit | ||
| from .frequency_units import frequency_units |
There was a problem hiding this comment.
Duplicate import of frequency_units detected. Remove the redundant import to clean up the module.
| from .frequency_units import frequency_units |
server/config.py
Outdated
| SECRET_KEY: str = Field(os.urandom(16).hex(), env="SECRET_KEY") | ||
| JWT_SECRET_KEY: str = Field(os.urandom(16).hex(), env="JWT_SECRET_KEY") |
There was a problem hiding this comment.
Using os.urandom at import time sets the same default each run and may not be re-evaluated per instance. Consider using default_factory=... to generate a new random key for each instantiation.
| SECRET_KEY: str = Field(os.urandom(16).hex(), env="SECRET_KEY") | |
| JWT_SECRET_KEY: str = Field(os.urandom(16).hex(), env="JWT_SECRET_KEY") | |
| def _generate_random_key() -> str: | |
| """Generate a random 16-byte key and return its hexadecimal representation.""" | |
| return os.urandom(16).hex() | |
| SECRET_KEY: str = Field(default_factory=_generate_random_key, env="SECRET_KEY") | |
| JWT_SECRET_KEY: str = Field(default_factory=_generate_random_key, env="JWT_SECRET_KEY") |
| @@ -0,0 +1,17 @@ | |||
| from enum import IntEnum | |||
|
|
|||
| class WavelengthUnits(IntEnum): | |||
There was a problem hiding this comment.
Astropy has a pretty extensive units package developed. Does it help us to use that?
| @@ -0,0 +1,35 @@ | |||
| from enum import IntEnum | |||
|
|
|||
| class Bandpass(IntEnum): | |||
There was a problem hiding this comment.
This feels very fragile. Should this be more dynamic and stored in the db?
server/utils/function.py
Outdated
| return False | ||
|
|
||
|
|
||
| def isFloat(s) -> bool: |
There was a problem hiding this comment.
These actually check if things are float. They only test if something can be cast to a float. We should probably at least change the name to better reflect that. Aslo python doesn't use camel case methods mostly. These functions feel like overkill? Is it not better to just have a try except when we are casting? This feels kind of non-Pythonic (read Javaesque). This kind of feels like trying to get around duck typing.
server/utils/function.py
Outdated
| Returns: | ||
| Projected footprint as a list of (ra, dec) tuples | ||
| """ | ||
| # Calculate center of the footprint |
There was a problem hiding this comment.
Should we use either the astropy wcs or reproject package to do these checks so we don't have to maintain this?
server/utils/function.py
Outdated
| chunks = [] | ||
| for i in range(0, len(items), n): | ||
| chunks.append(items[i : i + n]) | ||
| return chunks |
There was a problem hiding this comment.
Do we really need this function? It looks like we can just do this with index magic.
server/utils/function.py
Outdated
| try: | ||
| return float(i) | ||
| except: # noqa: E722 | ||
| return 0.0 |
There was a problem hiding this comment.
This scares me because 0.0 is number.
server/utils/function.py
Outdated
| Returns: | ||
| Tuple of (doi_id, doi_url) | ||
| """ | ||
| import uuid |
| config: Configuration object with credentials | ||
|
|
||
| Returns: | ||
| Filesystem object |
There was a problem hiding this comment.
Do we want to add support for local filesystems for development?
| central_wave, bandwidth, bandpass_enum | ||
| ) | ||
|
|
||
| freq_max = 2997924580000000000.0 / wave_min |
There was a problem hiding this comment.
I do not like these hardecoded numbers with lots of zeros Sam I Am. Can we use astropy constants?
| return SPEED_OF_LIGHT / wavelength_m | ||
|
|
||
|
|
||
| def freqToWave(freq: float) -> float: |
There was a problem hiding this comment.
It feels like a lot of these can be replaced with astropy unit conversions.
server/Dockerfile
Outdated
| WORKDIR /app | ||
|
|
||
| # Command to run the application | ||
| CMD ["uvicorn", "server.main:app", "--host", "0.0.0.0", "--port", "8000"] No newline at end of file |
tests/fastapi/test_admin.py
Outdated
| assert response.status_code == 401 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Does pytest really need this? Doesn't pytest basically automatically run as main by default?
tests/fastapi/test_candidate.py
Outdated
| assert candidate["associated_galaxy_redshift"] == 0.1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Same that I don't think we need a main.
| else: | ||
| # DOI creation failed (expected in test), so pointing won't be in DOI list | ||
| # This is acceptable - we've verified the endpoints work correctly | ||
| pass |
There was a problem hiding this comment.
Does this ever have the potential to mask a failure?
tests/fastapi/test_doi.py
Outdated
| assert response.status_code == status.HTTP_401_UNAUTHORIZED | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Main probably not necessary.
tests/fastapi/test_event.py
Outdated
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_gw_alert.py
Outdated
| assert False, f"Time field {time_field} is not in ISO 8601 format" | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_gw_galaxy.py
Outdated
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_health.py
Outdated
| assert "error" in redis | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_icecube.py
Outdated
| assert response.status_code == status.HTTP_401_UNAUTHORIZED | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_instrument.py
Outdated
| assert len([inst for inst in named_insts if inst["id"] in created_ids]) >= 3 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_query_alert.py
Outdated
| assert result[0]["prob_bns"] > 0.9 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
tests/fastapi/test_ui.py
Outdated
| assert response.status_code == 401 | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
| @@ -0,0 +1,11 @@ | |||
| # Test requirements for GWTM FastAPI tests | |||
There was a problem hiding this comment.
This should go into the pyproject.toml file. requirements.txt is no longer the standard.
| fastapi>=0.103.0 | ||
|
|
||
| # HTTP requests for API testing | ||
| requests>=2.31.0 No newline at end of file |
| fi | ||
| fi | ||
|
|
||
| echo "All tests completed successfully!" No newline at end of file |
| #!/bin/bash | ||
| # Run tests for a specific module | ||
| # Test data is loaded automatically by conftest.py | ||
| set -e |
There was a problem hiding this comment.
Isn't this just a pytest command? I don't really understand why we need an extra bash script here.
cmccully
left a comment
cmccully
left a comment
There was a problem hiding this comment.
I think overall this is tremendous work to get all of this migrated over to fast api and to get the tests all setup. I have made a variety of code readability/ maintainability comments.
Two main concerns:
How does this fit into the gwtm_api repo? Does this replace that repo?
I know we wanted to maintain backward compatibility originally, so we kept the Flask stuff in, but what is the plan to remove all the redundant code now that this is finished? That's a huge mental burden to leave all that code in, and I'd like to remove it while we remember what needs to be removed.
Fixes bugs, removes dead code & improves code quality based on reviewer feedback from Curtis, Austin, and Copilot. - Fix InstrumentType to instrument_type attribute references (3 files) - Delete unused test_refactoring.py and remove from router/README - Complete send_password_reset_email SMTP logic in email.py - Add fastapi to root requirements.txt - Improve skymap exception handling with proper logging - Reduce logging verbosity (DEBUG to INFO) - Remove pytest.main() blocks from all 11 test files - Use scientific notation in frequencyunits.py - Remove unused variables and unnecessary assignments - Replace bare except clauses with specific exception types - Add __all__ to models __init__.py - Remove unused imports (secrets, Path, io)
- Split function.py into focused modules (type_checks, geometry, doi, formatters) - Add AlertRole StrEnum for type-safe alert role comparisons - Refactor candidate filters to data-driven pattern with operator lookups - Replace grade calculator if/elif chains with threshold table lookups - Replace repeated probability rounding with getattr/setattr loop - Use strftime for date formatting in delete_test_alerts - Add local filesystem storage backend to gwtm_io - Add @contextmanager db_session() wrapper, delete unused db/utils.py - Migrate test deps to pyproject.toml, update CI workflow
Summary
• Implement complete FastAPI backend to replace legacy Flask application with modern async architecture
• Add comprehensive test coverage for all FastAPI endpoints with 11 test modules covering new code
Key Changes
New FastAPI Backend (server/)
Comprehensive Test Suite (tests/fastapi/)
Kubernetes Integration (gwtm-helm/)
This FastAPI implementation aims to maintain full API compatibility while providing better performance, automatic documentation, and type safety.