Skip to content

#66 loosen tolerance for comparing memory consumption #67

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 23, 2024
Merged

#66 loosen tolerance for comparing memory consumption #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
da30759
chore: move flake8-typing-imports to pip section
jschneidereit Jul 21, 2024
a7a1517
chore: add assertpy for readable test failures
jschneidereit Jul 21, 2024
a1e48da
chore: refactor java_heap tests for readable errors
jschneidereit Jul 21, 2024
56df799
fix: loosen tolerance for comparing memory consumption
jschneidereit Jul 21, 2024
fa38c52
Make the linter happy again
ctrueden Jul 23, 2024
eec97e8
Simplify tolerance reasoning in java_heap test
ctrueden Jul 23, 2024
b414c55
Test Pythons from 3.8 - 3.12
ctrueden Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ jobs:
]
python-version: [
'3.8',
'3.9',
'3.10'
'3.10',
'3.12'
]

steps:
Expand Down
3 changes: 2 additions & 1 deletion dev-environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ dependencies:
- numpy
- pandas
# Developer tools
- assertpy
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this package for this fix? I appreciate that it cleans up some of the test code for readability, however I would prefer to not add another dependency to scyjava.

Copy link
Member

Choose a reason for hiding this comment

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

@elevans If it were a transitive dependency for the main codebase I'd fully agree, but since it's only a developer dependency... I think I feel less strongly than you do about it. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I interest you in the power of readable test failure messages?

This is the original error message:

assert abs(mb_total - mb_initial) < 5, f"{mb_total=} {mb_initial=}"
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: mb_total=56 mb_initial=50

While this is th error message using assertpy:

AssertionError: [total mb should be close to initial] Expected <56> to be close to <50> within tolerance <5>, but was not.

The value becomes more clear when you write tests checking more complex things than number comparisons, take a scroll through their README to see if you recognize any patterns 😎

A more concrete example is near the end of test_arrays.py:

for i in range(len(nums_delta)):
    for j in range(len(nums_delta[i])):
        assert nums_delta[i][j] == pdoubles[i][j]

If you used assertpy you'd get a nice error message displaying both lists showing how they differ instead of just "false"

for i in range(len(nums_delta)):
    assert_that(nums_delta[i]).is_equal_to(pdoubles[i])

Copy link
Member

@elevans elevans Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Persuasion successful! Curtis is right and perhaps I was too against adding a new dependency. Since this would just be in the development config and not the main code base I have no problems here and in fact I think we could use assertpy in pyimagej and perhaps imglyb!

- autopep8
- black
- build
- flake8
- flake8-typing-imports
- isort
- pytest
- pytest-cov
Expand All @@ -42,5 +42,6 @@ dependencies:
- pip:
- git+https://github.com/ninia/jep.git@cfca63f8b3398daa6d2685428660dc4b2bfab67d
- flake8-pyproject
- flake8-typing-imports
- validate-pyproject[all]
- -e .
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ dependencies = [
[project.optional-dependencies]
# NB: Keep this in sync with dev-environment.yml!
dev = [
"assertpy",
"autopep8",
"black",
"build",
Expand Down
29 changes: 15 additions & 14 deletions tests/it/java_heap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
Test scyjava JVM memory-related functions.
"""

from assertpy import assert_that

import scyjava

mb_initial = 50 # initial MB of memory to snarf up
mb_tolerance = 10 # ceiling of expected MB in use

scyjava.config.set_heap_min(mb=mb_initial)
scyjava.config.set_heap_max(gb=1)
Expand All @@ -18,17 +21,15 @@
mb_total = scyjava.memory_total() // 1024 // 1024
mb_used = scyjava.memory_used() // 1024 // 1024

# Used memory should be less than the current memory total,
# which should be less than the maximum heap size.
assert mb_used <= mb_total <= mb_max, f"{mb_used=} {mb_total=} {mb_max=}"

# The maximum heap size should be approximately 1 GB.
assert 900 <= mb_max <= 1024, f"{mb_max=}"

# Most of that memory should still be free; i.e.,
# we should not be using more than a few MB yet.
assert mb_used <= 5, f"{mb_used=}"

# The total MB available to Java at this moment
# should be close to our requested initial amount.
assert abs(mb_total - mb_initial) < 5, f"{mb_total=} {mb_initial=}"
assert_that(
mb_used, "Used memory should be less than the current memory total"
).is_less_than_or_equal_to(mb_total)
assert_that(
mb_total, "current memory total should be less than maximum memory"
).is_less_than_or_equal_to(mb_max)
assert_that(mb_max, "maximum heap size should be approx. 1 GB").is_between(900, 1024)

assert_that(mb_used, "most memory should be available").is_less_than(mb_tolerance)
assert_that(mb_total, "total memory should be close to initial").is_close_to(
mb_initial, tolerance=mb_tolerance
)
Loading