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: MRR2 backend reattempt #198

Merged
merged 25 commits into from
Oct 16, 2024
Merged

ADD: MRR2 backend reattempt #198

merged 25 commits into from
Oct 16, 2024

Conversation

rcjackson
Copy link
Contributor

I have restarted the pull request for the MRR2 reader using the current version as a base. I'm hoping that I've made changes that fix the PR.

  • Closes #xxxx
  • Tests added
  • Changes are documented in history.md

@rcjackson
Copy link
Contributor Author

rcjackson commented Aug 23, 2024

Ruff is failing on notebooks unrelated to this PR. I wonder if there was a recent update for it to support Jupyter notebooks.

This reverts commit e377116.
* ADD: MRR2 backend

* Revert "ADD: MRR2 backend"

This reverts commit e377116.

* Fixed format to pass ruff.

* ADD: Documenting the changes in history.md

* FIX: Cmweather imports

* Cmweather oh cmweather

* ADD: MVC

* ADD: Link to PR in history.md
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 96.50794% with 11 lines in your changes missing coverage. Please review.

Project coverage is 92.26%. Comparing base (cd13dc1) to head (d4feaee).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
xradar/io/backends/metek.py 96.49% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
+ Coverage   91.97%   92.26%   +0.29%     
==========================================
  Files          23       24       +1     
  Lines        4573     4888     +315     
==========================================
+ Hits         4206     4510     +304     
- Misses        367      378      +11     
Flag Coverage Δ
notebooktests 78.70% <85.39%> (+0.46%) ⬆️
unittests 90.60% <95.87%> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@rcjackson rcjackson closed this Aug 23, 2024
@rcjackson rcjackson reopened this Aug 23, 2024
@kmuehlbauer
Copy link
Collaborator

@rcjackson Any idea on the bus error at the one nexrad test?

@rcjackson
Copy link
Contributor Author

rcjackson commented Aug 27, 2024 via email

Copy link
Collaborator

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

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

@rcjackson I've made a couple comments, how we might tackle the issue with the bus error.

I've checked the tests on main here:
https://github.com/openradar/xradar/actions/runs/10578167450
and they are working perfectly fine.

So I'm assuming that the ominous bus error is somehow introduced with the changes in this PR, or are revealed by the changes in this PR.

If you have any other ideas how to tackle this, please comment.

tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/io/test_metek.py Outdated Show resolved Hide resolved
@rcjackson
Copy link
Contributor Author

@kmuehlbauer None of the suggestions worked. I even explicitly close the datatree's Dataset to make sure the references clear, but that still doesn't seem to resolve the issue. I'll have to think of more solutions for this later, but for now I'm at a loss.

@kmuehlbauer
Copy link
Collaborator

@rcjackson This seems like a hard nut to crack, thanks for checking this out.

Maybe those open references are some hint as to why the error occurred in the first place? I'll have a look at our mrr reader (which is extremely fragile while being based on numpy.genfromtxt).

@kmuehlbauer
Copy link
Collaborator

@rcjackson We might try to run this sequentially.

Please change here:

pytest -n auto --dist loadfile --verbose --durations=15 --cov-report xml:coverage_unit.xml --cov=xradar --pyargs tests

and remove -n auto from the call.

Another thing to look after is this error:

  Traceback (most recent call last):
    File "/home/runner/work/xradar/xradar/xradar/io/backends/metek.py", line 447, in close
      for k in self._data.keys():
               ^^^^^^^^^^
  AttributeError: 'MRR2File' object has no attribute '_data'. Did you mean: 'data'?

which was issued here: https://github.com/openradar/xradar/actions/runs/10600167596/job/29376980199?pr=198

It seems, that one CI run finishes and all other raise with this error. Strange!

@rcjackson
Copy link
Contributor Author

@rcjackson We might try to run this sequentially.

Please change here:

pytest -n auto --dist loadfile --verbose --durations=15 --cov-report xml:coverage_unit.xml --cov=xradar --pyargs tests

and remove -n auto from the call.

Another thing to look after is this error:

  Traceback (most recent call last):
    File "/home/runner/work/xradar/xradar/xradar/io/backends/metek.py", line 447, in close
      for k in self._data.keys():
               ^^^^^^^^^^
  AttributeError: 'MRR2File' object has no attribute '_data'. Did you mean: 'data'?

which was issued here: https://github.com/openradar/xradar/actions/runs/10600167596/job/29376980199?pr=198

It seems, that one CI run finishes and all other raise with this error. Strange!

Running it sequentially seems to get it working, though slows down the testing a bit. Azure stores are typically memory limited at 1 GB, so I bet it was running in parallel and going up against the memory limit.

@kmuehlbauer
Copy link
Collaborator

Ok, so this looks like a memory leak somewhere.

This will need some profiling, I guess.

@kmuehlbauer kmuehlbauer enabled auto-merge (squash) October 16, 2024 11:25
@kmuehlbauer
Copy link
Collaborator

It seems, that finally there is a solution to the bus error issue.

Also this PR seems good enough to get in. Thanks @rcjackson, please feel free to make any additional changes in follow-up PR's.

@kmuehlbauer kmuehlbauer merged commit d06a145 into openradar:main Oct 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants