Skip to content

Enh/thrustcurve api cache#881

Merged
Gui-FernandesBR merged 4 commits intoRocketPy-Team:developfrom
IDL-IMTA-Students:enh/thrustcurve-api-cache
Nov 27, 2025
Merged

Enh/thrustcurve api cache#881
Gui-FernandesBR merged 4 commits intoRocketPy-Team:developfrom
IDL-IMTA-Students:enh/thrustcurve-api-cache

Conversation

@Monta120
Copy link
Copy Markdown
Contributor

@Monta120 Monta120 commented Nov 27, 2025

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Every call to GenericMotor.load_from_thrustcurve_api triggers a new download from the ThrustCurve API, even if the same motor has already been requested. This results in repeated network requests and slower test runs.

New behavior

  • Introduce caching for ThrustCurve API requests to avoid multiple downloads for the same motor.
  • Subsequent requests for the same motor now return cached data, improving efficiency and reducing network load.

Breaking change

  • No

@Monta120 Monta120 requested a review from a team as a code owner November 27, 2025 11:58
@Monta120 Monta120 changed the base branch from master to develop November 27, 2025 12:00
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.32%. Comparing base (9cf3dd4) to head (bd29470).
⚠️ Report is 8 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #881      +/-   ##
===========================================
+ Coverage    80.27%   80.32%   +0.05%     
===========================================
  Files          104      106       +2     
  Lines        12769    13001     +232     
===========================================
+ Hits         10250    10443     +193     
- Misses        2519     2558      +39     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

This way the cache will be deleted everytime you restart a python session. It doesn't solve 100% of the problem.

My idea is for us to create a .rocketpy_cache folder and save every download inside it. Before downloading a motor rom the API, we simply read it from file.

There should be options like "no_cache = Tre" to skip cache.

Please dedicate a few minutes to research how other python libraries tackle the same problem (caching downloads from rest API requests). I know OSMnx is a good example, but certainly there are many other libs doing the same thing.

Can you use these guidelines to improve this PR pls? I will mark it as a draft for now, but pls lemme know whener you finish it again so I can review it


Adittionaly, I think you have a problem with git tree, as I can see 43 commits on the commits section. I recommend you running the following: git rebase -i develop, then you select which commits you really need in this PR, and drop the others.

@Gui-FernandesBR Gui-FernandesBR marked this pull request as draft November 27, 2025 12:17
@Monta120 Monta120 force-pushed the enh/thrustcurve-api-cache branch from cc45dc7 to e6c0351 Compare November 27, 2025 13:54
@Monta120
Copy link
Copy Markdown
Contributor Author

Hi @Gui-FernandesBR , thank you for the feedback! I have implemented the changes you requested:

Persistent Caching: I updated the logic to use a persistent cache directory located at Path.home() / ".rocketpy_cache". The code now checks if the motor file exists locally before making a network request, persisting data across Python sessions.

no_cache Option: I added a no_cache=False argument to load_from_thrustcurve_api. Setting it to True forces a fresh download from the API.

I also ensured that the unit tests use a temporary directory for the cache (via monkeypatch) so they don't clutter the user's home folder during testing.

@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review November 27, 2025 14:03
@Gui-FernandesBR
Copy link
Copy Markdown
Member

Hi @Gui-FernandesBR , thank you for the feedback! I have implemented the changes you requested:

Persistent Caching: I updated the logic to use a persistent cache directory located at Path.home() / ".rocketpy_cache". The code now checks if the motor file exists locally before making a network request, persisting data across Python sessions.

no_cache Option: I added a no_cache=False argument to load_from_thrustcurve_api. Setting it to True forces a fresh download from the API.

I also ensured that the unit tests use a temporary directory for the cache (via monkeypatch) so they don't clutter the user's home folder during testing.

Amazing! You're a star @Monta120 !!

Copy link
Copy Markdown
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

LGTM, only need a final touch on the CHANGELOG file.

And let's wait for copilot's review as well.

Great work

@github-project-automation github-project-automation bot moved this from Backlog to Next Version in LibDev Roadmap Nov 27, 2025
@Gui-FernandesBR Gui-FernandesBR linked an issue Nov 27, 2025 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

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 adds caching functionality to the ThrustCurve API integration to avoid redundant network requests when loading the same motor multiple times. The implementation uses a file-based cache stored in ~/.rocketpy_cache and introduces a no_cache parameter to allow bypassing the cache when needed.

Key Changes

  • Introduced automatic caching of ThrustCurve API responses to avoid repeated downloads
  • Added no_cache parameter to force fresh API downloads when needed
  • Implemented comprehensive test coverage for caching behavior

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
rocketpy/motors/motor.py Adds cache directory initialization, implements caching logic in _call_thrustcurve_api, and adds no_cache parameter to load_from_thrustcurve_api
tests/unit/motors/test_genericmotor.py Adds new test test_thrustcurve_api_cache to verify caching works correctly, including cache reads, writes, and bypass functionality
docs/user/motors/genericmotor.rst Updates documentation to explain caching behavior and the no_cache parameter with examples
CHANGELOG.md Documents the new caching feature and related changes
Comments suppressed due to low confidence (1)

rocketpy/motors/motor.py:2018

  • The no_cache parameter is missing from the docstring. According to NumPy docstring conventions, all function parameters should be documented in the "Parameters" section. Add documentation for the no_cache parameter, including its type (bool), default value (False), and description.
    def load_from_thrustcurve_api(name: str, no_cache: bool = False, **kwargs):
        """
        Creates a Motor instance by downloading a .eng file from the ThrustCurve API
        based on the given motor name.

        Parameters
        ----------
        name : str
            The motor name according to the API (e.g., "Cesaroni_M1670" or "M1670").
            Both manufacturer-prefixed and shorthand names are commonly used; if multiple
            motors match the search, the first result is used.
        **kwargs :
            Additional arguments passed to the Motor constructor or loader, such as
            dry_mass, nozzle_radius, etc.

@Gui-FernandesBR
Copy link
Copy Markdown
Member

@Monta120 pls mark all the comments as "solved" after solving it, so we can merge it directly

@Monta120
Copy link
Copy Markdown
Contributor Author

I'm working on testing exception handling for cache operations to ensure above 80.27% coverage.

@Gui-FernandesBR Gui-FernandesBR merged commit ae46e55 into RocketPy-Team:develop Nov 27, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from Next Version to Closed in LibDev Roadmap Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Closed

Development

Successfully merging this pull request may close these issues.

Cache thrust curve API requests

3 participants