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

Feat/cache queries #21

Merged
merged 17 commits into from
Nov 22, 2024
Merged

Feat/cache queries #21

merged 17 commits into from
Nov 22, 2024

Conversation

JensZack
Copy link
Collaborator

Cache results from _get_id
In the run of:
r2x -i temp_data/oct1024_Con_NG --input-model=reeds-US --output-model=plexos --weather-year=2012 -o .out/$CASETS --year 2035 --plugins pcm_defaults emission_cap break_gens imports --debug --upgrade --flags daily-budgets=true

In this case:
_get_id is called 434408 times
there are only 5397 unique calls of _get_id

Caching _get_id results leads to the performance gains:
plexos_exporter: 47.3s -> 19.7s
_get_id cumulative: 28.8s -> 1.7s

Added:
  - Function to create an integer for query_string/param input
  - _QUERY_CACHE object to hold results from all queries
@JensZack JensZack changed the title Feat/cache queries Draft: Feat/cache queries Nov 18, 2024
Some queries do change throught the process of using plexosdb from r2x, get_id should not change during a run
 - Added flag to turn cache on/off
 - Added clear_id_cache method
 - Moved all cache methods to bottom of class
- Enforce unique name/class for all objects
- Enforce unqiue object ids for all objects
- Raise ValueError if an object is added without a unique name/class
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@24c1a0b). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #21   +/-   ##
=======================================
  Coverage        ?   97.14%           
=======================================
  Files           ?        5           
  Lines           ?      596           
  Branches        ?        0           
=======================================
  Hits            ?      579           
  Misses          ?       17           
  Partials        ?        0           

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


🚨 Try these New Features:

@JensZack JensZack changed the title Draft: Feat/cache queries Feat/cache queries Nov 21, 2024
@JensZack
Copy link
Collaborator Author

I would like to clean up the cacheing mechanism to only use class/object name, for id queries in the object table. This also raises questions about what uniqueness can be enforced in other tables where _get_id is used repeatedly.

Copy link
Collaborator

@pesap pesap left a comment

Choose a reason for hiding this comment

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

LGTM. We can merge once the comments are addressed.

src/plexosdb/sqlite.py Outdated Show resolved Hide resolved
tests/data/plexosdb.xml Show resolved Hide resolved
tests/test_plexosdb_sqlite.py Show resolved Hide resolved
tests/test_plexosdb_sqlite.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pesap pesap left a comment

Choose a reason for hiding this comment

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

Can you also improve the coverage to at least 92%? I think we are missing like 7 lines of coverage. I think it is from the cache implementation however the CI is not showing where. We will need to run coverage command locally.

tests/test_plexosdb_sqlite.py Show resolved Hide resolved
src/plexosdb/sqlite.py Outdated Show resolved Hide resolved
@pesap pesap self-requested a review November 21, 2024 20:39
Copy link
Collaborator

@pesap pesap left a comment

Choose a reason for hiding this comment

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

Can you run the coverage with this command locally:

coverage run -m pytest -vvl && coverage report -m --sort

tests/test_plexosdb_sqlite.py Outdated Show resolved Hide resolved
@pesap pesap self-requested a review November 22, 2024 15:29
@JensZack JensZack merged commit 0f52e9b into main Nov 22, 2024
7 checks passed
@JensZack JensZack deleted the feat/cache-queries branch November 22, 2024 19:15
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.

3 participants