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: track memoization stats #51

Merged
merged 27 commits into from
Feb 6, 2024
Merged

feat: track memoization stats #51

merged 27 commits into from
Feb 6, 2024

Conversation

WinPlay02
Copy link
Contributor

@WinPlay02 WinPlay02 commented Feb 2, 2024

Closes partially #44

Summary of Changes

  • track more stats: last access, computation time, last access time, memory usage
  • refactored memoization logic into a new class
  • updated to safe-ds library 0.19

Copy link

github-actions bot commented Feb 2, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 3 0 0 0.77s
✅ PYTHON mypy 3 0 2.29s
✅ PYTHON ruff 3 0 0 0.03s
✅ REPOSITORY git_diff yes no 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ee959d) 100.00% compared to head (b501fce) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #51   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          436       499   +63     
=========================================
+ Hits           436       499   +63     

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

WinPlay02 added a commit to WinPlay02/Library that referenced this pull request Feb 5, 2024
This is needed to correctly evaluate whether an object is worth memoizing or keeping in the cache.

See for context: Safe-DS/Runner#51 and Safe-DS/Runner#44
lars-reimann pushed a commit to Safe-DS/Library that referenced this pull request Feb 5, 2024
Summary:
- feat: return the correct size for custom container objects

This is needed to correctly evaluate whether an object is worth
memoizing or keeping in the cache.

See for context: Safe-DS/Runner#51 and
Safe-DS/Runner#44

For future container classes (like e.g. image set this would also need
to be added, to be compatible with the memoizing implementation in the
runner)
@WinPlay02
Copy link
Contributor Author

I would propose to merge the tracking of stats (this PR) first and continue the strategies for memoizing and removing in a follow-up PR.

WinPlay02 and others added 5 commits February 6, 2024 14:54
…nner into memoization-expansion

# Conflicts:
#	src/safeds_runner/server/memoization_map.py
test: update memory usage test to relative comparison, as the size is not constant across python versions
@WinPlay02 WinPlay02 changed the title feat: memoization expansion feat: memoization expansion part 1 Feb 6, 2024
@WinPlay02 WinPlay02 marked this pull request as ready for review February 6, 2024 14:24
@WinPlay02 WinPlay02 requested a review from a team as a code owner February 6, 2024 14:24
@lars-reimann
Copy link
Member

I would propose to merge the tracking of stats (this PR) first and continue the strategies for memoizing and removing in a follow-up PR.

I'm all for that.

src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
tests/safeds_runner/server/test_memoization.py Outdated Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
src/safeds_runner/server/memoization_map.py Outdated Show resolved Hide resolved
@lars-reimann lars-reimann changed the title feat: memoization expansion part 1 feat: track memoization stats Feb 6, 2024
WinPlay02 and others added 5 commits February 6, 2024 16:55
feat: track multiple stats per function instead of replacing them

test: import directly, if possible
…nner into memoization-expansion

# Conflicts:
#	src/safeds_runner/server/memoization_map.py
lars-reimann
lars-reimann previously approved these changes Feb 6, 2024
Copy link
Member

@lars-reimann lars-reimann left a comment

Choose a reason for hiding this comment

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

I've made some changes that were easier to implement directly than to describe them.

@WinPlay02 Please take a quick look whether I broke anything and then feel free to merge if you're happy with it (or complain if you are not).

@WinPlay02
Copy link
Contributor Author

I've made some changes that were easier to implement directly than to describe them.

@WinPlay02 Please take a quick look whether I broke anything and then feel free to merge if you're happy with it (or complain if you are not).

This should be good to go

@lars-reimann lars-reimann merged commit 50f30a3 into main Feb 6, 2024
8 checks passed
@lars-reimann lars-reimann deleted the memoization-expansion branch February 6, 2024 19:09
lars-reimann pushed a commit that referenced this pull request Feb 8, 2024
## [0.6.0](v0.5.0...v0.6.0) (2024-02-08)

### Features

* track memoization stats ([#51](#51)) ([50f30a3](50f30a3)), closes [#44](#44)
@lars-reimann
Copy link
Member

🎉 This PR is included in version 0.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lars-reimann lars-reimann added the released Included in a release label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Included in a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants