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

Add memray support #7376

Closed

Conversation

WilliamJamieson
Copy link
Collaborator

@WilliamJamieson WilliamJamieson commented Dec 2, 2022

This PR adds usage of the pytest-memray plugin to the tests. This is part of the effort to begin tracking memory usage in JWST (and other packages, see SCSB-57.

Some questions:

  • Should we turn pytest-memray on by default? Unfortunately, it currently is not supported by Windows and will cause the unit tests to fail during collection if it is used there.
  • Does using pytest-memray need documentation somewhere in the JWST docs?
  • Should we start decorating some of the tests with memory usage limits, see the pytest-memray docs? (Maybe this should be done in a series of follow-up PRs)

In a future version of pytest-memray there should be support for catching memory leaks, see bloomberg/pytest-memray#52. This maybe very useful for catching future memory issues.

Note that until bloomberg/pytest-memray#60 is resolved, we should not merge this PR.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@github-actions github-actions bot added automation Continuous Integration (CI) and testing automation tools installation testing labels Dec 2, 2022
@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (041c730) 76.56% compared to head (fd86e1c) 76.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7376   +/-   ##
=======================================
  Coverage   76.56%   76.56%           
=======================================
  Files         456      456           
  Lines       36877    36877           
=======================================
  Hits        28234    28234           
  Misses       8643     8643           
Flag Coverage Δ *Carryforward flag
nightly 77.43% <ø> (ø) Carriedforward from 041c730

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -78,6 +78,7 @@ test =
pytest>=6.0.0
pytest-cov>=2.9.0
pytest-doctestplus>=0.10.0
pytest-memray
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this require a version number?

Copy link
Collaborator

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

LGTM, failing CI are from the tests that fail on Python 3.11

@stscieisenhamer
Copy link
Collaborator

Unless there are cases/reasons to check memory usage during CI, I would think we do not want it on by default. However, if this feature can be test-specific, developing some canary-in-the-mine tests would be useful.

@nden nden closed this Jul 2, 2023
@nden nden reopened this Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Continuous Integration (CI) and testing automation tools installation testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants