Skip to content

Conversation

alvarolopez
Copy link
Collaborator

@alvarolopez alvarolopez commented Jun 28, 2024

V3 development PR.

Fixes #99

Copy link

Copy link

sonarqubecloud bot commented Aug 8, 2024

Copy link

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@alvarolopez alvarolopez force-pushed the v3-fastapi branch 3 times, most recently from 16e1ad0 to 6965b58 Compare March 21, 2025 21:36
@alvarolopez alvarolopez requested a review from Copilot September 26, 2025 09:55
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates DEEPaaS from aiohttp to FastAPI as the web framework backend, marking a major version bump to 3.0.0. The migration removes training functionality and modernizes the API architecture.

  • Replaces aiohttp/aiohttp-apispec with FastAPI for HTTP handling and OpenAPI generation
  • Removes training endpoint support entirely across the codebase
  • Updates Python version requirements and dependency management

Reviewed Changes

Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pyproject.toml Updates version to 3.0.0, replaces aiohttp with fastapi dependencies
deepaas/api/ Complete rewrite of API modules to use FastAPI routing and responses
deepaas/model/v2/ Removes training methods and simplifies model wrapper architecture
deepaas/cmd/run.py Switches from aiohttp web server to uvicorn for FastAPI
deepaas/tests/ Updates test suite to work with new FastAPI-based architecture
doc/ Updates documentation to reflect removal of training endpoints

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

assume its a custom field"""
ftype = type(field)
if issubclass(ftype, marshmallow.fields.Field) and ftype not in FIELD_CONVERTERS:
print(" Custom field")
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

Debug print statements should be removed from production code. Use logging instead or remove these statements entirely.

Copilot uses AI. Check for mistakes.

def is_file_field(field):
"""If this is a file field, we need to handle it differently."""
if field is not None and field.metadata.get("type") == "file":
print(" File field")
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

Debug print statements should be removed from production code. Use logging instead or remove these statements entirely.

Copilot uses AI. Check for mistakes.

"""Compare endpoints in both OpenAPI specs.
This method ignoring methods that have been removed.
"""
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

The function accesses dictionary keys without checking if 'paths' key exists, which could raise a KeyError if the OpenAPI spec is malformed. Add validation to ensure required keys exist in the spec.

Suggested change
"""
"""
# Validate that 'paths' key exists in both specs
if 'paths' not in old_spec:
raise ValueError("The old OpenAPI spec is missing the 'paths' key.")
if 'paths' not in new_spec:
raise ValueError("The new OpenAPI spec is missing the 'paths' key.")

Copilot uses AI. Check for mistakes.

Comment on lines +73 to +75
# FIXME(aloga): Validation does not work, as we are converting from
# Marshmallow to Pydantic, check this as son as possible.
# self.model_obj.validate_response(ret)
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

Response validation is disabled which could allow invalid responses to be returned to clients. This should be fixed before production use as it bypasses important data validation.

Copilot uses AI. Check for mistakes.

Comment on lines +74 to +76
f"Old: {old_response.status_code}, New: {new_response.status_code}"
)
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

There's a typo in the comment on line 75 - 'son' should be 'soon' in the FIXME comment above.

Copilot uses AI. Check for mistakes.

We use this function to be able to include the router in the main
application and do things before it gets included.
In this case we explicitly include the model precit endpoint.
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

Typo in docstring: 'precit' should be 'predict'.

Suggested change
In this case we explicitly include the model precit endpoint.
In this case we explicitly include the model predict endpoint.

Copilot uses AI. Check for mistakes.

@alvarolopez alvarolopez force-pushed the v3-fastapi branch 2 times, most recently from 7eaf30a to 219bdc6 Compare September 26, 2025 10:22
Copy link

sonarqubecloud bot commented Sep 29, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
4 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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.

Refactor code to remove v2 organization

2 participants