dssg / triage

General Purpose Risk Modeling and Prediction Toolkit for Policy and Social Good Problems
Other
182 stars 61 forks source link

update wrapt version #882

Closed tweddielin closed 2 years ago

tweddielin commented 2 years ago

Upgrade wrapt package to 1.13.3 to fix the issue #863.

shaycrk commented 2 years ago

@tweddielin -- seems like good news/bad news... apparently bumping the numpy version up to 1.22.1 fixes the python 3.9 error we've been getting here but at the expense of losing python 3.7 support. That's not ideal since I think many of our current projects have been using python 3.7, but we certainly want to be able to work with newer postgres versions.

I guess the other potential direction here is to remove the wrapt dependency entirely and try to implement the getstate/setstate approach we discussed? Thoughts?

BTW, converting the PR to a draft for the time being since I've been playing with the python version support...

shaycrk commented 2 years ago

Ok -- here's another option: using environment markers to specify different numpy versions for different python versions. I don't think this is a great long-term solution, but maybe a reasonable bandaid to get things working until we're ready to let go of python 3.7 support?

tweddielin commented 2 years ago

@shaycrk looks like all tests pass with matplotlib removal from conftest.py

shaycrk commented 2 years ago

Thanks @tweddielin -- makes sense. Should we try switching back to the single version of numpy in that case?

shaycrk commented 2 years ago

Hmmm... looks like with the uniform version of numpy, we still get the same error, even with removing the matplotlib import in conftest.py:

==================================== ERRORS ====================================
________________________ ERROR collecting test session _________________________
.tox/py3/lib/python3.9/site-packages/_pytest/config/__init__.py:495: in _importconftest
    return self._conftestpath2mod[key]
E   KeyError: PosixPath('/home/runner/work/triage/triage/src/tests/conftest.py')

During handling of the above exception, another exception occurred:
.tox/py3/lib/python3.9/site-packages/py/_path/local.py:704: in pyimport
    __import__(modname)
.tox/py3/lib/python3.9/site-packages/_pytest/assertion/rewrite.py:152: in exec_module
    exec(co, module.__dict__)
src/tests/conftest.py:4: in <module>
    from tests.utils import sample_config, populate_source_data
src/tests/utils.py:9: in <module>
    import matplotlib
.tox/py3/lib/python3.9/site-packages/matplotlib/__init__.py:207: in <module>
    _check_versions()
.tox/py3/lib/python3.9/site-packages/matplotlib/__init__.py:192: in _check_versions
    from . import ft2font
E   ImportError: numpy.core.multiarray failed to import

---------- coverage: platform linux, python 3.9.10-final-0 -----------

=========================== short test summary info ============================
ERROR  - ImportError: numpy.core.multiarray failed to import
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 1.04s ===============================
ERROR: InvocationError for command /home/runner/work/triage/triage/.tox/py3/bin/py.test --basetemp=/home/runner/work/triage/triage/.tox/py3/tmp -vvv --cov=triage (exited with code 2)
___________________________________ summary ____________________________________
ERROR:   py3: commands failed
Error: Process completed with exit code 1.

Looks like it's now running into a matplotlib import issue in test.utils now causing the same problem. Will go back to the two versions of numpy for the time being, but @tweddielin not sure if you want to keep digging on the root cause of the error here?

tweddielin commented 2 years ago

@shaycrk looks like updating matplotlib to 3.3.4 works

shaycrk commented 2 years ago

Interesting -- thanks @tweddielin! Strange that it was causing that problem, but glad that works and can avoid the divergent numpy versions!

shaycrk commented 2 years ago

Just tested with a fresh ec2 instance and appears to work with both python 3.7 and 3.8 across postgres 11, 12, and 13. What's confusing is that I'm now getting the pickling error with triage 5.0.0 (wrapt 1.12.1) and postgres 11.12 (the same version as our main database), which wasn't happening before in this testing and we haven't seen with other projects...

Regardless, the wrapt update seems like it solves this problem, so we should probably go ahead and pull it in and tag a bugfix version. Changing back to a standard PR for review.

thcrock commented 2 years ago

@shaycrk When you assigned it to me did you intend for me to merge it once I approve?

shaycrk commented 2 years ago

Thanks @thcrock! Either way is fine with me -- I'll go ahead and merge it now and tag a version for pypi.