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

Update/pydantic v2 #217

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Update/pydantic v2 #217

wants to merge 16 commits into from

Conversation

gmorales96
Copy link

@gmorales96 gmorales96 commented Dec 24, 2024

Description:

This PR migrates the project to Pydantic v2 and drops support for Python 3.8

Key Changes:

  1. Updates cuenca-validations to version 2.0.0 which includes Pydantic v2 support
  2. Upadtes fastapi to version 0.115.6
  3. Dropped Support for Python 3.8

Breaking Changes:

  • Code relying on Pydantic v1 API require updates to work with the new version.

Summary by CodeRabbit

  • Python Version Update

    • Updated project Python version from 3.8 to 3.13
    • Updated GitHub Actions workflows to support Python 3.9-3.13
    • Updated Makefile and setup configuration
  • Dependency Updates

    • Updated multiple dependencies in requirements.txt and requirements-test.txt
    • Added new dependencies like httpx
    • Upgraded testing and development tools
  • Code Improvements

    • Enhanced error handling in middleware and resource configurations
    • Updated type annotations for consistency across the codebase
    • Modernized Pydantic model configurations and validation mechanisms
  • Testing Enhancements

    • Added new test cases for SQS tasks, including validation for invalid JSON messages
    • Improved test coverage and configuration across various modules
    • Adjusted test execution context for SQS-related tests
  • Version Update

    • Incremented software version from '0.14.1' to '2.0.0'
  • Configuration Update

    • Added new configuration option for pytest to handle asynchronous tests

gabino added 2 commits December 24, 2024 11:09
- Update cuenca-validations from 0.11.19 to 2.0.0.dev7 and fastapi from 0.68.2 to 0.115.6 in requirements.txt.
- Modify QueryResponse model in rest_api.py to use Field for descriptions.
- Add a new test for handling invalid JSON messages in test_sqs_tasks.py.
Copy link

coderabbitai bot commented Dec 24, 2024

Walkthrough

This pull request introduces a comprehensive update to the project's Python version and dependency management. The changes span multiple files across the project, focusing on upgrading the Python version from 3.8 to 3.13 and updating various dependencies. The modifications include updates to GitHub Actions workflows for both release and testing processes, the Makefile, requirements files, and setup configuration. The project's test infrastructure has been expanded to support multiple Python versions (3.9 through 3.13), and several dependencies have been updated to their latest versions. Additionally, minor code refactoring and type annotation improvements were made across various files, including middleware configurations, resource handling, and validator setups. The version number of the software has also been updated from '0.14.1' to '2.0.0', indicating a significant version increment.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cef192 and 1af26cf.

📒 Files selected for processing (1)
  • requirements.txt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • requirements.txt

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gmorales96 gmorales96 requested a review from pachCode December 24, 2024 17:34
@gmorales96 gmorales96 self-assigned this Dec 24, 2024
@gmorales96 gmorales96 closed this Dec 31, 2024
@gmorales96 gmorales96 reopened this Jan 4, 2025
Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (67e4b41) to head (1af26cf).

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #217      +/-   ##
===========================================
+ Coverage   99.46%   100.00%   +0.53%     
===========================================
  Files          10        10              
  Lines         374       375       +1     
  Branches       53        34      -19     
===========================================
+ Hits          372       375       +3     
+ Misses          2         0       -2     
Flag Coverage Δ
unittests 100.00% <100.00%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmorales96 gmorales96 marked this pull request as ready for review January 4, 2025 03:11
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (8)
examples/resources/base.py (1)

22-22: Consistent usage of return type hint

Similar to the previous function, using -> None aligns well with the actual behavior of raising an exception. If you want to signal that the function never returns under any condition, NoReturn can be kept instead, but it's a stylistic choice.

examples/app.py (1)

21-21: Consider adding typed stubs

While adding # type: ignore removes type checker errors, it can obscure real issues. If possible, provide a typed stub or direct type hint for FastAgaveErrorHandler to avoid having to ignore type checking.

setup.py (1)

26-31: Reevaluate strict version pinning

Pinning exact versions (e.g., ==2.16.1) ensures a consistent environment but prevents receiving potential security patches or minor updates automatically. Consider using a more flexible version range if stability allows.

examples/validators.py (1)

29-35: Consider adding or verifying request body validations.
While json_schema_extra provides an illustrative example, ensure the AccountRequest fields have validators as needed (e.g., string length, regex). This will align with Pydantic’s new approach to model-specific constraints without a separate Config class.

examples/middlewares/authed.py (1)

17-27: Refactor star-arg unpacking after a keyword argument.
According to the static analysis hint (B026), star-arg unpacking after a keyword argument can reduce clarity. Consider refactoring so that all positional arguments come before keyword arguments.

 def __init__(
     self, app, plugins=None, default_error_response=None, *args, **kwargs
 ):
     super().__init__(
-        app=app,
-        plugins=plugins,
-        default_error_response=default_error_response,
-        *args,
-        **kwargs,
+        app,
+        *args,
+        plugins=plugins,
+        default_error_response=default_error_response,
+        **kwargs,
     )
🧰 Tools
🪛 Ruff (0.8.2)

24-24: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

tests/conftest.py (1)

189-190: Consider using a context manager for the Moto server.
While explicitly calling start() and stop() works fine, a context manager might simplify the server lifecycle.

Also applies to: 196-196, 197-197

tests/tasks/test_sqs_tasks.py (1)

394-394: Potential performance overhead.
A 5-second sleep can slow tests. If feasible, consider reducing or mocking out the delay to speed up the test suite while still verifying concurrency logic.

fast_agave/blueprints/rest_api.py (1)

233-233: Ensure correct file naming.
The content-disposition header is built correctly to include an extension guess. Keep an eye on non-common MIME types to avoid incorrect naming.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67e4b41 and 305ebaf.

📒 Files selected for processing (17)
  • .github/workflows/release.yml (1 hunks)
  • .github/workflows/test.yml (4 hunks)
  • Makefile (1 hunks)
  • examples/app.py (1 hunks)
  • examples/middlewares/authed.py (1 hunks)
  • examples/resources/base.py (1 hunks)
  • examples/resources/users.py (1 hunks)
  • examples/tasks/retry_task_example.py (1 hunks)
  • examples/validators.py (4 hunks)
  • fast_agave/blueprints/rest_api.py (3 hunks)
  • fast_agave/tasks/sqs_tasks.py (2 hunks)
  • requirements-test.txt (1 hunks)
  • requirements.txt (1 hunks)
  • setup.py (1 hunks)
  • tests/blueprint/test_decorators.py (1 hunks)
  • tests/conftest.py (2 hunks)
  • tests/tasks/test_sqs_tasks.py (5 hunks)
✅ Files skipped from review due to trivial changes (3)
  • tests/blueprint/test_decorators.py
  • examples/tasks/retry_task_example.py
  • requirements.txt
🧰 Additional context used
🪛 Ruff (0.8.2)
examples/middlewares/authed.py

24-24: Star-arg unpacking after a keyword argument is strongly discouraged

(B026)

🔇 Additional comments (25)
examples/resources/users.py (1)

21-21: Prevent NoneType attribute errors

By checking api_request.client before accessing api_request.client.host, you've ensured that user.ip is safely set to None if no client info is present. This is a good defensive coding practice.

examples/resources/base.py (1)

17-17: Ensure type annotation consistency

These functions always raise exceptions and do not return any value, so using -> None is acceptable. You may keep NoReturn to indicate a function that never returns normally, but there's no significant runtime impact either way.

setup.py (2)

24-24: Aligned Python requirement with project upgrade

Raising the minimum required Python version to 3.9 makes sense, reflecting the removal of support for Python 3.8.


34-38: Broadened classifier support

Great job specifying explicit support for Python 3.9 through 3.13, aligning with the upgraded environment and expanded compatibility.

examples/validators.py (3)

4-4: Great choice to import ConfigDict for Pydantic v2 support.
This change helps keep your configuration in sync with the new Pydantic style.


61-67: Confirm the AccountUpdateRequest model’s business logic.
Make sure that the AccountUpdateRequest only includes the fields to be updated (e.g., name). If more fields are required in future, update json_schema_extra examples accordingly.


Line range hint 45-56: Retain the same naming conventions for consistency.
The field names in AccountResponse are consistent with the rest of the codebase. Verify all references to AccountResponse reflect the new json_schema_extra usage.

✅ Verification successful

Let me verify the usage of AccountResponse and its schema consistency by checking the related request model and actual response data.


Let me try a different approach to verify the schema consistency.


Field names in AccountResponse are consistent with the codebase

After analyzing the codebase, I can confirm that:

  • The field names in AccountResponse (id, name, user_id, platform_id, created_at, deactivated_at) perfectly match the fields in the Account model
  • The response schema is correctly used in the accounts resource where AccountResponse is set as the response_model
  • The field naming convention follows the same snake_case pattern used throughout other models in the codebase
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Example script to locate code references to AccountResponse
rg -A 3 $'AccountResponse'

Length of output: 609


Script:

#!/bin/bash
# Check AccountRequest model definition and its schema
ast-grep --pattern 'class AccountRequest'

# Check actual response construction in the create endpoint
ast-grep --pattern 'async def create(request: AccountRequest) -> Response:'

Length of output: 121


Script:

#!/bin/bash
# Search for any Pydantic models and their schema definitions
rg -B 2 -A 6 "class.*BaseModel.*:"

# Check the actual response construction in the create endpoint
rg -B 2 -A 6 "async def create.*AccountRequest.*:"

Length of output: 7363

fast_agave/tasks/sqs_tasks.py (2)

11-11: Approved import of validate_call.
Switching from validate_arguments to validate_call aligns with Pydantic v2’s recommended validators for dynamic function argument validation.


109-109: Verify migrations from validate_arguments to validate_call.
Ensure all references to task_func that relied on old Pydantic v1 features are updated. If you had custom validators or constraints, confirm they still work as intended.

✅ Verification successful

Let me gather more information about the validation usage and any potential custom validators.


Let me check for any task function definitions and their parameters to understand the validation context better.


Let me check one final thing to see how the tasks are being used in the examples.


Based on the gathered information, I can now provide a final response:

Migration to validate_call appears to be clean and complete
The codebase shows a clean migration to Pydantic v2's validate_call. There are no traces of the old validate_arguments usage, and the validation setup appears to be properly integrated with the task system. The Pydantic models in the codebase are following v2 patterns (using ConfigDict for configuration), suggesting a consistent upgrade across the codebase.

  • The validation is applied uniformly through the task decorator system in fast_agave/tasks/sqs_tasks.py
  • All Pydantic models in the examples and tests are using v2-compatible patterns
  • No legacy validate_arguments imports or usage were found
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
rg -A 3 'validate_call'

Length of output: 520


Script:

#!/bin/bash
# Check for any previous validate_arguments usage
rg -A 3 'validate_arguments'

# Look for custom validator decorators and validation related code
rg -A 3 '@validator|@root_validator'

# Check the full context of the task function and validation
ast-grep --pattern 'task_with_validators = validate_call($_)'

Length of output: 225


Script:

#!/bin/bash
# Look for task function definitions
ast-grep --pattern 'def $_($$$) -> $$$'

# Check for any Pydantic model definitions that might be used for validation
rg -A 5 'class.*BaseModel'

# Look for the full context of where task_func is defined or passed
rg -B 3 -A 3 'task_func'

Length of output: 7030


Script:

#!/bin/bash
# Look for task usage examples and their validation
rg -A 5 '@task.*def'

# Check for any imports of validate_arguments to confirm complete migration
fd -e py -x cat {} | rg 'from.*validate_arguments'

Length of output: 77

tests/conftest.py (2)

187-188: Switching to ThreadedMotoServer is a solid improvement.
This approach offers simpler code over subprocess and more direct control of the test environment.


228-232: Enabling ContentBasedDeduplication is a great step for FIFO queues.
Double-check that any existing deduplication logic or message grouping logic aligns with this setting.

tests/tasks/test_sqs_tasks.py (4)

111-111: Confirm compatibility with Pydantic v2.
Invoking model_dump() ensures the data is correctly serialized for the mock function call. This aligns with Pydantic v2 usage and looks good.


267-269: Docstring references inclusive changes to max retries.
The inline documentation now correctly states max_retries=2 and explains that the task will execute three times in total.


288-291: Validate new retry behavior.
The updated max_retries=2 parameter and corresponding expected calls ([call(test_message)] * 3) align well with the docstring. Ensure tests pass consistently for under/over invocation scenarios.


411-441: Well-implemented invalid JSON test.
The new test adequately checks that invalid messages are ignored and removed without executing the task. This is a good addition for robust error handling.

fast_agave/blueprints/rest_api.py (3)

10-10: Appropriate import usage for Pydantic v2.
Including Field and ValidationError directly from pydantic clarifies usage and improves readability.


257-259: Clearer query description.
The multi-line string clarifies parameters and pagination usage for clients. Looks good.


264-275: Refined field definitions using Field.
Leveraging Field for QueryResponse clarifies each attribute’s role and description. This approach is consistent with Pydantic v2 best practices.

requirements-test.txt (3)

1-3: Dependency updates align with Python 3.13 upgrade.
Upgrading black, flake8, and isort is consistent with keeping the codebase current.


6-9: Notable changes for testing stack.
Updating moto[server], mypy, pytest, and pytest-cov ensures better coverage and type-checking for Python 3.13.


13-13: New dependency added.
Adding httpx==0.28.1 can enhance testing of async HTTP interactions. Confirm that related mocks or usage are covered in tests.

.github/workflows/release.yml (2)

10-10: Consistent setup for release workflow.
Using actions/[email protected] is up-to-date with GitHub’s recommended action versions.


13-13: Upgraded to Python 3.13.
This aligns with the PR objective to drop support for Python 3.8 and adopt Pydantic v2. Ensure all external dependencies support 3.13 for a smooth release.

.github/workflows/test.yml (2)

53-53: LGTM! Good security practice.

Adding the Codecov token as a secret is a good security practice for coverage reporting.


23-23: Verify Python version compatibility with Pydantic v2

The Python version matrix looks good for dropping Python 3.8 support. However, we should verify the minimum Python version required by Pydantic v2.

✅ Verification successful

Python version matrix is compatible with Pydantic v2 requirements

The Python version matrix in the workflow (3.9 to 3.13) is fully compatible with Pydantic v2's minimum requirement of Python >=3.8. The current configuration exceeds the minimum requirement, ensuring proper compatibility.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check Pydantic v2's Python version requirements
curl -s https://pypi.org/pypi/pydantic/json | jq -r '.info.requires_python'

Length of output: 81

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
examples/tasks/task_example.py (1)

31-31: Pydantic v2 migration: model_dump() usage looks good.

The change from dict() to model_dump() correctly aligns with Pydantic v2's API changes.

Since you're dropping Python 3.8 support, you could modernize the type hints using the union operator:

-async def task_validator(message: Union[User, Company]) -> None:
+async def task_validator(message: User | Company) -> None:
fast_agave/tasks/sqs_client.py (1)

4-4: Modernize type hints.

Since Python 3.8 support is being dropped, you could modernize all type hints in this file.

-from typing import Optional, Union
+from typing import Optional  # Remove if not used elsewhere in the file

 async def send_message(
     self,
-    data: Union[str, dict],
-    message_group_id: Optional[str] = None,
+    data: str | dict,
+    message_group_id: str | None = None,
 ) -> None:

 def send_message_async(
     self,
-    data: Union[str, dict],
-    message_group_id: Optional[str] = None,
+    data: str | dict,
+    message_group_id: str | None = None,
 ) -> asyncio.Task:

Also applies to: 40-42, 51-53

requirements-test.txt (1)

11-11: Consider updating pytest-asyncio version.

The current version constraint pytest-asyncio==0.18.* is relatively old. Latest versions (0.23+) offer better support for Python 3.9+ and newer pytest versions.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 305ebaf and d98fa3e.

📒 Files selected for processing (16)
  • .github/workflows/release.yml (1 hunks)
  • .github/workflows/test.yml (3 hunks)
  • examples/app.py (1 hunks)
  • examples/resources/base.py (1 hunks)
  • examples/resources/cards.py (1 hunks)
  • examples/tasks/task_example.py (1 hunks)
  • examples/validators.py (4 hunks)
  • fast_agave/blueprints/rest_api.py (5 hunks)
  • fast_agave/filters.py (2 hunks)
  • fast_agave/tasks/sqs_celery_client.py (3 hunks)
  • fast_agave/tasks/sqs_client.py (3 hunks)
  • requirements-test.txt (1 hunks)
  • requirements.txt (1 hunks)
  • tests/blueprint/test_blueprint.py (4 hunks)
  • tests/conftest.py (9 hunks)
  • tests/tasks/test_sqs_tasks.py (12 hunks)
✅ Files skipped from review due to trivial changes (1)
  • tests/blueprint/test_blueprint.py
🚧 Files skipped from review as they are similar to previous changes (7)
  • examples/app.py
  • examples/resources/base.py
  • fast_agave/blueprints/rest_api.py
  • requirements.txt
  • tests/conftest.py
  • .github/workflows/test.yml
  • examples/validators.py
🧰 Additional context used
🪛 Ruff (0.8.2)
fast_agave/filters.py

5-5: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: pytest (3.13)
  • GitHub Check: pytest (3.12)
  • GitHub Check: pytest (3.11)
  • GitHub Check: pytest (3.10)
  • GitHub Check: coverage
  • GitHub Check: pytest (3.9)
🔇 Additional comments (7)
fast_agave/filters.py (1)

20-20: Pydantic v2 migration: model_dump() usage looks good.

The change from dict() to model_dump() correctly aligns with Pydantic v2's API changes.

examples/resources/cards.py (1)

22-22: Type hint update looks good.

The change from Dict to dict correctly aligns with PEP 585 (Python 3.9+).

fast_agave/tasks/sqs_celery_client.py (1)

5-5: Type hint updates look good!

The changes from Dict to dict align with modern Python type hinting practices and Pydantic v2 recommendations for using built-in types.

Let's verify type hint consistency across the codebase:

Also applies to: 12-12, 63-63, 72-72

✅ Verification successful

Type hint changes are consistent across the codebase

The migration from Dict to dict type hints has been completed consistently. No remaining instances of typing.Dict type hints were found, and existing type hints follow the modern Python style using built-in types.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining Dict type hints that should be updated
rg "Dict\[" --type py

Length of output: 23


Script:

#!/bin/bash
# Check for any remaining Dict imports and usage
rg "from typing import.*Dict" --type py
rg "Dict[^\\[]" --type py
# Check current dict type hint usage for comparison
rg "dict\[" --type py

Length of output: 316

.github/workflows/release.yml (1)

9-13: GitHub Actions configuration updates look good!

The changes appropriately:

  • Update Python version to 3.13
  • Pin actions to stable versions (v4, v5, v1)

Also applies to: 20-20

tests/tasks/test_sqs_tasks.py (3)

111-111: Good update to use model_dump()!

The change to use model_dump() aligns with Pydantic v2's API changes, replacing the deprecated dictionary conversion methods.


412-441: Well-structured test for invalid JSON handling!

The new test case properly verifies that:

  1. Invalid JSON messages are handled gracefully
  2. Messages are removed from the queue
  3. Tasks are not executed for invalid messages

393-394: Review the increased sleep duration in concurrency test.

The sleep duration has been increased from 1 to 5 seconds, which could significantly impact test execution time. Consider if this duration is necessary for reliable test results.

Let's check if there are any reported test timing issues:

✅ Verification successful

Sleep duration in concurrency test is appropriate

The 5-second duration is justified as it ensures reliable concurrent task measurement in test_concurrency_controller. This test specifically verifies that the maximum number of concurrent tasks never exceeds 2 while processing 5 messages, requiring sufficient task overlap time for accurate measurement.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for timing-related issues or discussions in the codebase
rg -i "timing|flaky|intermittent" --type md

Length of output: 45


Script:

#!/bin/bash
# First, let's see the full context of this test
cat tests/tasks/test_sqs_tasks.py

# Also search for similar sleep patterns in test files
rg "asyncio.sleep" --type py tests/

Length of output: 13477

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/conftest.py (1)

187-197: Consider using a dynamic port for ThreadedMotoServer.

The hardcoded port 4000 could cause conflicts in parallel test execution environments. Consider using a dynamic port allocation strategy.

-    server = ThreadedMotoServer(port=4000)
+    server = ThreadedMotoServer()  # Let the OS assign an available port
     server.start()
 
     endpoints = dict(
-        sqs='http://127.0.0.1:4000/',
+        sqs=f'http://127.0.0.1:{server.port}/',
     )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0bb0cad and 4cef192.

📒 Files selected for processing (8)
  • fast_agave/version.py (1 hunks)
  • requirements.txt (1 hunks)
  • setup.cfg (1 hunks)
  • setup.py (1 hunks)
  • tests/conftest.py (10 hunks)
  • tests/tasks/test_sqs_celery_client.py (0 hunks)
  • tests/tasks/test_sqs_client.py (0 hunks)
  • tests/tasks/test_sqs_tasks.py (11 hunks)
💤 Files with no reviewable changes (2)
  • tests/tasks/test_sqs_celery_client.py
  • tests/tasks/test_sqs_client.py
✅ Files skipped from review due to trivial changes (1)
  • fast_agave/version.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • setup.py
  • tests/tasks/test_sqs_tasks.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: pytest (3.13)
  • GitHub Check: pytest (3.12)
  • GitHub Check: pytest (3.11)
  • GitHub Check: pytest (3.10)
  • GitHub Check: coverage
  • GitHub Check: pytest (3.9)
🔇 Additional comments (6)
setup.cfg (1)

6-6: LGTM! The asyncio_mode setting aligns with Pydantic v2 migration.

The addition of asyncio_mode = auto is appropriate for a FastAPI project migrating to Pydantic v2, as it will properly handle both synchronous and asynchronous tests automatically.

Let's verify that the test suite runs correctly with this new setting:

✅ Verification successful

Verified: asyncio_mode = auto is appropriate for the async test suite

The configuration is well-suited for this codebase as it properly handles:

  • Async test functions in SQS-related tests
  • Async fixtures using aiobotocore
  • Mixed sync/async test environment
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any async test files that might be affected by this change

# Look for async test functions
echo "Checking for async test functions:"
rg -l 'async\s+def\s+test' tests/

# Look for pytest.mark.asyncio decorators
echo "Checking for pytest.mark.asyncio usage:"
rg -l '@pytest.mark.asyncio' tests/

Length of output: 338


Script:

#!/bin/bash
# Check content of one async test file to understand the testing pattern
echo "Content of test_sqs_client.py:"
cat tests/tasks/test_sqs_client.py

# Look for any existing pytest asyncio configuration
echo -e "\nChecking for existing pytest asyncio config:"
rg -l "asyncio" tests/conftest.py setup.cfg setup.py pytest.ini 2>/dev/null

# Look for conftest.py files
echo -e "\nLooking for conftest.py files:"
fd conftest.py

Length of output: 1438


Script:

#!/bin/bash
# Check content of conftest.py to verify fixture setup
echo "Content of conftest.py:"
cat tests/conftest.py

Length of output: 6585

tests/conftest.py (3)

5-5: LGTM! Type imports updated for Python 3.9+ compatibility.

The changes align with modern Python type hinting practices and the migration away from Python 3.8.

Also applies to: 14-14


52-52: LGTM! Fixture type annotations updated to PEP 585 standards.

Type hints have been consistently updated across all fixtures to use built-in collection types (PEP 585), which is required for Python 3.9+ compatibility.

Also applies to: 94-94, 99-99, 104-104, 110-110, 120-120, 126-126, 152-152, 158-158, 167-167


228-232: Consider adding tests for message deduplication.

The addition of ContentBasedDeduplication is good, but should be accompanied by tests that verify the deduplication behavior.

Would you like me to help create test cases that verify the message deduplication functionality?

requirements.txt (2)

2-2: ⚠️ Potential issue

Avoid using development versions in production.

The cuenca-validations==2.0.0.dev14 is a development version which may be unstable or contain breaking changes. Consider using a stable release version instead.

Let's check if there's a stable release available:


1-1: Breaking changes alert: Major version updates.

The following dependencies have major version updates which likely include breaking changes:

  • aiobotocore: 2.1.0 → 2.17.0
  • mongoengine-plus: 0.0.3 → 1.0.0

Please ensure that your codebase is compatible with these new versions.

Let's check for any breaking changes in the changelog:

Also applies to: 4-4

Copy link
Member

@felipao-mx felipao-mx left a comment

Choose a reason for hiding this comment

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

solo un comentario

@@ -14,6 +14,17 @@


class AuthedMiddleware(ContextMiddleware):
def __init__(
Copy link
Member

Choose a reason for hiding this comment

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

remover este init no veo que haga algo extra

Copy link
Author

Choose a reason for hiding this comment

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

BaseHTTPMiddleware cambió en versiones recientes de Starlette. Ahora el parámetro app debe pasarse explícitamente como argumento al inicializar un middleware basado en BaseHTTPMiddleware.

De lo contrario obtenemos: BaseHTTPMiddleware.__init__() missing 1 required positional argument: 'app'

Podemos hacer algo más simple como:

    def __init__(self, app):
        super().__init__(app=app)

Sería mejor?

@gmorales96 gmorales96 requested a review from felipao-mx January 21, 2025 15:38
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