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

fix: Remove WeakList in favor of built-ins #1530

Merged
merged 16 commits into from
Aug 24, 2021

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Jul 21, 2021

Description

Resolves #1506

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Rewrite the events code to handle weakrefs more properly
   - This is needed to work with Click v8.0+
* Fix up tests that were incorrectly written due to API changes in unittest.mock

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Jul 22, 2021

I think something is going wrong with the conftest(?) as I can trigger the errors seen in doctest locally with

$ python -m pytest -x -r sx --ignore tests/benchmarks/ --ignore tests/contrib --ignore tests/test_notebooks.py
src/pyhf/infer/utils.py ..                                                                                                                                                                          [  3%]
src/pyhf/tensor/jax_backend.py F

================================================================================================ FAILURES =================================================================================================
_________________________________________________________________________ [doctest] pyhf.tensor.jax_backend.jax_backend.astensor __________________________________________________________________________
201 
202         Convert to a JAX ndarray.
203 
204         Example:
205 
206             >>> import pyhf
207             >>> pyhf.set_backend("jax")
UNEXPECTED EXCEPTION: TypeError("'NoneType' object is not callable")
Traceback (most recent call last):
  File "/home/feickert/.pyenv/versions/3.8.6/lib/python3.8/doctest.py", line 1336, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest pyhf.tensor.jax_backend.jax_backend.astensor[1]>", line 1, in <module>
  File "/home/feickert/Code/GitHub/pyhf/src/pyhf/events.py", line 122, in register_wrapper
    result = func(*args, **kwargs)
  File "/home/feickert/Code/GitHub/pyhf/src/pyhf/__init__.py", line 147, in set_backend
    events.trigger("tensorlib_changed")()
  File "/home/feickert/Code/GitHub/pyhf/src/pyhf/events.py", line 53, in __call__
    func()(*args, **kwargs)
TypeError: 'NoneType' object is not callable
/home/feickert/Code/GitHub/pyhf/src/pyhf/tensor/jax_backend.py:207: UnexpectedException

but

$ pytest -x --doctest-modules src/pyhf/infer/utils.py src/pyhf/tensor/numpy_backend.py src/pyhf/tensor/jax_backend.py

runs fine.

@matthewfeickert matthewfeickert force-pushed the fix/change-weaklist-to-callable-for-click branch from 2ed43bc to c92ed63 Compare August 18, 2021 14:01
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #1530 (ad9baf7) into master (29c3df0) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1530      +/-   ##
==========================================
+ Coverage   97.66%   97.70%   +0.03%     
==========================================
  Files          63       63              
  Lines        4024     4050      +26     
  Branches      572      576       +4     
==========================================
+ Hits         3930     3957      +27     
+ Misses         55       54       -1     
  Partials       39       39              
Flag Coverage Δ
contrib 25.40% <27.27%> (-0.02%) ⬇️
unittests 97.48% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/events.py 100.00% <100.00%> (+2.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29c3df0...ad9baf7. Read the comment docs.

@kratsg
Copy link
Contributor

kratsg commented Aug 20, 2021

Just putting a note that coverage needs to be fixed, but this is confirmed to work by @alexander-held .

@matthewfeickert
Copy link
Member Author

Just putting a note that coverage needs to be fixed, but this is confirmed to work by @alexander-held .

Once this is in and we're happy with it working let's cut a v0.6.3 release so that this won't keep hitting cabinetry and the rest of our users. 👍

Copy link
Member Author

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

I'm sure that I'm missing the obvious here, but if this isn't already being hit anywhere in CI then what are the types of events that would trigger this?

The removal still passes CI and the following

# /tmp/test.py
import pyhf

model = pyhf.simplemodels.uncorrelated_background(
    signal=[5.0], bkg=[10.0], bkg_uncertainty=[2.0]
)

pyhf.set_backend("jax", "minuit")

model = pyhf.simplemodels.uncorrelated_background(
    signal=[5.0], bkg=[10.0], bkg_uncertainty=[2.0]
)

src/pyhf/events.py Show resolved Hide resolved
src/pyhf/events.py Show resolved Hide resolved
@kratsg kratsg marked this pull request as ready for review August 24, 2021 14:29
Copy link
Member Author

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Highest of fives @kratsg. This is great. :)

Also thanks for these tests as this better shows me how to do things with Mock. 👍

@matthewfeickert matthewfeickert merged commit 4e0f92e into master Aug 24, 2021
@matthewfeickert matthewfeickert deleted the fix/change-weaklist-to-callable-for-click branch August 24, 2021 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

annoying error messages when exiting script using pyhf Exceptions related to weakref in pyhf 0.6.2
2 participants