Python lint #133
Python lint #133
Conversation
✅ Deploy Preview for mdv-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors and modernizes several components. The SQLAlchemy model definitions now use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProjectService
participant DBSession
Client->>ProjectService: Call add_new_project(path, name)
Note over ProjectService: Instantiate empty Project object
ProjectService->>ProjectService: Assign 'name' and 'path' attributes
ProjectService->>DBSession: Commit Project object
DBSession-->>ProjectService: Confirmation
sequenceDiagram
participant Developer
participant ServerDebug
Developer->>ServerDebug: Run debug script (server_debug.py)
ServerDebug->>FlaskApp: Initialize app in debug mode (port 5056)
FlaskApp-->>ServerDebug: Listening on port 5056
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
python/mdvtools/project_router.py (1)
206-206:⚠️ Potential issueFix import position.
The
functoolsimport should be moved to the top of the file with the other imports.-import functools +Then add it to the top imports section:
1 from flask import Flask, jsonify 2 from flask.typing import ResponseReturnValue as Response 3 from typing import Dict, Any, Callable, Tuple 4 import re 5 from urllib.parse import urlparse 6 from datetime import datetime, timedelta +import functools 7🧰 Tools
🪛 GitHub Actions: Python CI
[warning] 206-206: Module level import not at top of file.
python/mdvtools/dbutils/test/integration/test_dbservice_integration.py (1)
3-3:⚠️ Potential issueMissing dependency in project requirements.
The
flask_testingmodule is imported but missing from project requirements, causing CI pipeline failures.Add this dependency to your project's requirements file:
# requirements.txt or setup.py + flask-testing🧰 Tools
🪛 GitHub Check: pyright (3.12, 1.1.343)
[failure] 3-3:
Import "flask_testing" could not be resolved (reportMissingImports)🪛 GitHub Actions: Python CI
[error] 3-3: ModuleNotFoundError: No module named 'flask_testing'.
🧹 Nitpick comments (4)
python/mdvtools/dbutils/test/integration/test_dbservice_integration.py (2)
99-100: Dual type safety assertions.Using both
assertIsNotNoneandassert(file is not None)is redundant. Consider standardizing on one approach throughout the test file.- self.assertIsNotNone(file) # is there a way to make the type checker understand this? - assert(file is not None) + self.assertIsNotNone(file) # This assertion is sufficient
138-174: Vertical spacing inconsistency.There's inconsistent vertical spacing between the File object creations. Consider standardizing the spacing for better readability.
file1 = File() file1.name='file1.txt' file1.file_path='/files/project/file1.txt' file1.project_id=project.id file1.upload_timestamp=datetime.now() file1.update_timestamp=datetime.now() - + file2 = File() file2.name='file2.txt' file2.file_path='/files/project/file2.txt' file2.project_id=project.id file2.upload_timestamp=datetime.now() file2.update_timestamp=datetime.now()python/mdvtools/dbutils/test/unit/test_dbservice.py (2)
94-94: Redundant assertion.You already perform
self.assertIsNotNone(result)above; this inlineassertduplicates that check. Consider removing for cleaner tests.- assert(result is not None)
105-105: Redundant assertion.Same as earlier suggestion; prefer a single
self.assertIsNotNoneover inline assert.- assert(result is not None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
python/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
python/mdvtools/dbutils/dbmodels.py(2 hunks)python/mdvtools/dbutils/dbservice.py(3 hunks)python/mdvtools/dbutils/server_debug.py(1 hunks)python/mdvtools/dbutils/test/integration/test_dbservice_integration.py(8 hunks)python/mdvtools/dbutils/test/unit/test_dbservice.py(4 hunks)python/mdvtools/mdvproject.py(2 hunks)python/mdvtools/project_router.py(4 hunks)python/mdvtools/server.py(12 hunks)python/mdvtools/test_projects/dtypes_test.py(2 hunks)python/mdvtools/test_projects/genome_example.py(1 hunks)python/mdvtools/tests/test_anndata_conversion.py(1 hunks)python/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- python/mdvtools/test_projects/genome_example.py
🧰 Additional context used
🪛 GitHub Actions: Python CI
python/mdvtools/project_router.py
[warning] 206-206: Module level import not at top of file.
python/mdvtools/dbutils/test/unit/test_dbservice.py
[error] 3-3: ModuleNotFoundError: No module named 'flask_sqlalchemy'.
python/mdvtools/dbutils/test/integration/test_dbservice_integration.py
[error] 3-3: ModuleNotFoundError: No module named 'flask_testing'.
python/mdvtools/server.py
[warning] 28-28: datetime.timedelta imported but unused. Remove unused import: datetime.timedelta.
python/mdvtools/dbutils/dbmodels.py
[error] 1-1: Import "flask_sqlalchemy" could not be resolved (reportMissingImports)
🪛 GitHub Check: pyright (3.12, 1.1.343)
python/mdvtools/dbutils/dbmodels.py
[failure] 3-3:
Import "sqlalchemy" could not be resolved (reportMissingImports)
[failure] 4-4:
Import "sqlalchemy.orm" could not be resolved (reportMissingImports)
🔇 Additional comments (70)
python/pyproject.toml (1)
68-68:❓ Verification inconclusive
Good addition of flask-testing for test infrastructure.
Adding the flask-testing library is appropriate for supporting test cases with the refactored SQLAlchemy models. This will help ensure that the modernized models using
DeclarativeBaseandmapped_columnare properly tested.
🏁 Script executed:
#!/bin/bash # Verify if flask-testing is compatible with the current Flask version # Check the Flask version requirement in pyproject.toml FLASK_VERSION=$(rg -A 1 "Flask" python/pyproject.toml | grep -o '".*"' | head -1) echo "Current Flask version requirement: $FLASK_VERSION" # Check if the flask-testing version is appropriate for this Flask version pip install flask-testing==0.8.1 pip show flask-testing | grep -A 5 "Requires"Length of output: 2478
Flask-Testing Dependency Addition Verified: Confirm Flask Version Compatibility
The inclusion of
flask-testing(version 0.8.1) is a well-aligned addition for bolstering the test infrastructure for the modernized SQLAlchemy models. However, note that while yourpyproject.tomldefines Flask as"3.0.3", installingflask-testingpulled Flask 3.1.0. Please verify that this minor version difference does not lead to any compatibility issues with the rest of your backend dependencies.python/mdvtools/tests/test_anndata_conversion.py (1)
12-12: Excellent path resolution improvement.This change improves the robustness of the test by making the path resolution relative to the test file's location rather than the current working directory. This ensures that the test can find the test datasets regardless of where the test is executed from, which is a best practice for portable and reliable tests.
python/mdvtools/mdvproject.py (2)
1822-1822: Improved type safety in category check.Converting
data.dtypeto a string before comparison with "category" is more robust. This handles cases wheredata.dtypeis an object with string representation "category" rather than a string literal, preventing potential type-related bugs.
1876-1876: Better type checking order in string validation.Reordering the conditions to check
isinstance(v, str)before checking ifvis empty is a good defensive programming practice. This ensures we only perform string operations on actual string objects, preventing potential type errors.python/mdvtools/dbutils/server_debug.py (1)
1-59: Well-documented debugging utility for local development.This new debugging utility makes it easier for developers to run and debug the MDV server locally. The extensive documentation of the Safari-specific issues and troubleshooting steps provides valuable context for anyone encountering similar problems in the future.
python/mdvtools/project_router.py (4)
2-2: Good use of new typing module import.The import change from
flasktoflask.typingforResponseReturnValuedemonstrates good practice for improved type safety.
25-25: Improved function signature with return type annotation.Adding the return type annotation
-> Responseenhances type safety and code clarity.
86-89: Clear and informative docstring for ProjectBlueprint_v2.Good addition of documentation explaining the purpose and differences of this class.
122-123: Helpful implementation note.This comment effectively communicates the implementation difference between the two blueprint classes.
python/mdvtools/dbutils/dbservice.py (3)
37-37: Improved default project name.Changed default from 'unnamed_project' to 'unnamed project' for better readability.
39-42: Improved type-checking compatibility.Refactored object construction to use attribute assignment rather than constructor parameters, which improves type-checking compatibility as noted in the comment.
175-180: Consistent object creation pattern.Changed
Fileinstantiation to use the same pattern as forProjectobjects, improving type-checking and consistency across the codebase.python/mdvtools/test_projects/dtypes_test.py (4)
6-6: Good replacement of os with tempfile.Using
tempfileinstead ofosis a better choice for test environments.
35-40: Helpful documentation for test purpose.Adding this comment clarifies that this is not an actual test but a project setup script.
41-52: Improved test isolation with TemporaryDirectory.Using a temporary directory for the test project improves test isolation and prevents filesystem conflicts.
140-140: Properly positioned set_view call.Moved the
set_viewcall inside the TemporaryDirectory context, ensuring proper project setup.python/mdvtools/dbutils/test/integration/test_dbservice_integration.py (3)
29-30: Type checking suppressions for backward compatibility.The
# type: ignorecomments are a good temporary solution for maintaining backward compatibility with both old and new SQLAlchemy models.
90-93: Consistent object creation pattern in tests.Changed
Projectinstantiation to match the new pattern used in service methods, improving type-checking compatibility.
116-122: Consistent File object creation pattern.This instantiation pattern matches the new approach in the service code, providing better type safety.
python/mdvtools/dbutils/test/unit/test_dbservice.py (21)
33-34: Patching Project query fromdbservicelooks correct.
39-40: Updated patch references for Project and session.
49-49: Consistent patch reference forProject.query.
55-56: Switching patch targets todbserviceusage is valid.
63-64: Refactoring patch for Project creation.
70-70: Aligningtest_get_project_by_id_failureto updated import path.
76-77: Refined patch targets for soft-delete flow.
86-87: Patch references for FileService binding.
97-98: Patch references for file update scenario.
108-108: UsingFile.queryfromdbservice.
114-114: References forfile_exists_in_project_success.
120-120: References forget_files_by_project_success.
126-127: Deleting files indbservicecontext.
135-136: Updated patch forFile.queryand session handling.
145-146: Patch references for file add/update error condition.
153-154: Patch references for file update scenario (failure).
162-162: References forget_file_by_path_and_project_failure.
168-168: References forfile_exists_in_project_failure.
174-174: References forget_files_by_project_failure.
180-181: File deletion failure: updated patch references.
188-189: Timestamp update failure: patching new references.python/mdvtools/server.py (17)
12-12: ImportingResponseReturnValuefor type annotations.
133-133: Added return type annotation forproject_index.
142-142: Added-> ResponseReturnValuetoget_binary_file.
152-152: Type annotation forget_binary_file_gz.
158-158: Added-> ResponseReturnValuetoget_json_file.
168-168: Added-> ResponseReturnValuetoget_data.
185-185: Added return type annotation toimagesroute.
193-193:get_configsnow annotated withResponseReturnValue.
198-198:get_viewusesResponseReturnValuefor clarity.
206-206: Added return type annotation toget_row_data.
222-222:get_binary_dataupdated to returnResponseReturnValue.
245-245: Routesend_trackwith typed return.
253-253:save_datatype annotation for response.
329-329: Explicit return type foradd_anndata.
420-420:combine_anndataadds a typed return.
527-527: Adding-> ResponseReturnValuetoadd_or_update_image_datasource.
560-560:add_datasourceroute annotated withResponseReturnValue.python/mdvtools/dbutils/dbmodels.py (13)
3-5: Switch to modern SQLAlchemy imports withMappedandmapped_column.🧰 Tools
🪛 GitHub Check: pyright (3.12, 1.1.343)
[failure] 3-3:
Import "sqlalchemy" could not be resolved (reportMissingImports)
[failure] 4-4:
Import "sqlalchemy.orm" could not be resolved (reportMissingImports)
7-8: DeclarativeBase usage.
10-10:SQLAlchemy(model_class=Base): Great for typed models.
14-27: User model typed columns.
All look consistent with the declared column types.
58-67: File model typed columns are consistent.
70-74: UserProject columns typed properly.
78-89: Genome model typed columns.
All consistent with usage.
93-103: Job model typed columns.
107-110: Permission model typed columns.
114-119: SharedObject columns.
Looks consistent with usage.
123-126: UserPreference: typed columns with JSON for data.
130-142: ViewSet typed columns, withonupdate=datetime.now.
146-153: GeneSet typed columns.
Aligns well with the pattern.
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
| name: Mapped[str] = mapped_column(String(255), nullable=False, default='unnamed_project') | ||
| path: Mapped[str] = mapped_column(String(1024), nullable=False, unique=True) | ||
| created_timestamp: Mapped[datetime] = mapped_column(DateTime, nullable=False, default=datetime.now) | ||
| is_deleted: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) | ||
| deleted_timestamp: Mapped[datetime] = mapped_column(DateTime, nullable=True) | ||
| update_timestamp: Mapped[datetime] = mapped_column(DateTime, nullable=False, default=datetime.now) | ||
| accessed_timestamp: Mapped[datetime] = mapped_column(DateTime, nullable=False, default=datetime.now) | ||
| owner: Mapped[int] = mapped_column(Integer) | ||
| type: Mapped[str] = mapped_column(Text) | ||
| data: Mapped[Any] = mapped_column(JSON) | ||
| is_public: Mapped[bool] = mapped_column(Boolean, nullable=False, default=False) | ||
| date_made_public: Mapped[datetime] = mapped_column(DateTime) | ||
| status: Mapped[str] = mapped_column(Text) | ||
| genome: Mapped[str] = mapped_column(String, ForeignKey('genomes.name')) | ||
| parent: Mapped[str] = mapped_column(Integer) | ||
| description: Mapped[str] = mapped_column(Text) | ||
| access_level: Mapped[str] = mapped_column(String(50), nullable=False, default='editable') # Default access level | ||
| users = relationship('UserProject', backref='project', lazy=True) | ||
| files = relationship('File', backref='project', lazy=True) |
There was a problem hiding this comment.
Project model typed columns.
Be mindful of line 46:
parent: Mapped[str] = mapped_column(Integer)The type annotation (str) conflicts with the column type (Integer). This may cause runtime confusion.
- parent: Mapped[str] = mapped_column(Integer)
+ parent: Mapped[int] = mapped_column(Integer)
|
@coderabbitai a big issue we have with the project currently is that browsing to a project with Safari when running python/mdvtools/dbutils/mdv_server_app.py will stall the server. I've described some of the symptoms and steps taken to understand the issue in python/mdvtools/dbutils/server_debug.py. We should probably have a github issue for that... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll need to look at the server_debug.py file to understand the issue better before discussing the GitHub issue. 🏁 Script executed: Length of output: 4914 @xinaesthete I've examined the server_debug.py file and understand the issue. This looks like an SSL/TLS negotiation problem specific to Safari. The garbled binary data in the logs are TLS handshake packets that the server isn't handling properly. Would you like me to create a GitHub issue documenting this problem? I can include:
Let me know if you'd like me to create this issue, and if there's any additional information you'd like included. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@xinaesthete I've examined the server_debug.py file and understand the issue. This looks like an SSL/TLS negotiation problem specific to Safari. The garbled binary data in the logs are TLS handshake packets that the server isn't handling properly. Would you like me to create a GitHub issue documenting this problem? I can include:
Let me know if you'd like me to create this issue, and if there's any additional information you'd like included. 🏁 Script executed: Length of output: 4914 @xinaesthete I've analyzed the server_debug.py file and understand the issue you're experiencing. This is definitely an SSL/TLS negotiation problem specific to Safari. The garbled binary data in the logs are TLS handshake packets that the server is misinterpreting as HTTP requests. Would you like me to create a GitHub issue documenting this problem? I can include:
Let me know if you'd like me to create this issue, and if there's any additional information you'd want included. |
@coderabbitai open a github issue for this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@xinaesthete I'll create a GitHub issue for the Safari SSL/TLS handshake problem now. @xinaesthete I've created a GitHub issue documenting the Safari SSL/TLS handshake problem that's causing the server to stall. The issue includes:
The issue has been assigned to you for tracking. Let me know if you'd like any additional information added to help with troubleshooting this problem. 🏁 Script executed: Length of output: 2225 @xinaesthete The GitHub issue has been successfully created! You can find it at: #139 I've documented the Safari SSL/TLS handshake problem with detailed information about:
Would you like me to also do a follow-up review of the PR changes? If so, I can focus on any specific aspects you'd like me to evaluate. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@xinaesthete I've created a GitHub issue documenting the Safari SSL/TLS negotiation problem. The issue includes:
You'll be automatically notified of the issue since I've tagged you in it. 🏁 Script executed: Length of output: 2276 @xinaesthete I've successfully created a GitHub issue documenting the Safari SSL/TLS negotiation problem. The issue (#140) is now available at: The issue includes detailed information about:
You've been tagged in the issue and will receive notifications for any updates. |
Some refactor of db models to use more modern
DeclarativeBase/mapped_columnetc, see https://flask-sqlalchemy.readthedocs.io/en/stable/models/Some other changes to better satisfy type-checking.
Added
server_debug.pythat can help with running server in debugger locally.Summary by CodeRabbit
New Features
Refactor
Chores