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

Possible fix for your broken unittests #1573

Open
tblanarik opened this issue Feb 7, 2023 · 11 comments
Open

Possible fix for your broken unittests #1573

tblanarik opened this issue Feb 7, 2023 · 11 comments

Comments

@tblanarik
Copy link

tblanarik commented Feb 7, 2023

I don't know if you've come across this yet, but I think the following breaking change to pint is also responsible for your broken unittests:

You can be reasonably sure because the last test that succeeds (794) shows:

  • Successfully installed pint-0.18

while the first to break shows:

  • Successfully installed pint-0.19.1

In addition, the stacktrace shows this is happening while unpickling solar_13.p:

  File "C:\Users\jenkins\workspace\CE_gpkit_Push_unit_tests\buildnode\windows10x64\optimizer\cvxopt\docs\source\examples\breakdowns.py", line 11, in <module>
    sol = pickle.load(open(dirpath+"solar_13.p", "rb"))
  File "C:\Users\jenkins\workspace\CE_gpkit_Push_unit_tests\buildnode\windows10x64\optimizer\cvxopt\venv_jenkins\lib\site-packages\pint\util.py", line 429, in __setstate__
    self._d, self._one, self._non_int_type = state
ValueError: too many values to unpack (expected 3)

I haven't had a chance to look around the project much yet (I just started by trying to run the unittests), but it seems like the fix is maybe just to update rtd_requirements.txt to read:

pint == 0.18
adce

If/when I am confident, I can also submit a PR to that effect.

@tblanarik
Copy link
Author

tblanarik commented Feb 7, 2023

What's strange is that the rest of the builds seem to be pinned at pint==0.9, which is why they haven't broken

eg: https://acdl.mit.edu/csi/view/convex%20engineering/job/CE_gpkit_Push_unit_tests/buildnode=macys_VM,optimizer=cvxopt/lastBuild/console

@whoburg
Copy link
Collaborator

whoburg commented Feb 21, 2023

@tblanarik! Great to hear from you! Thanks for the possible fix, great sleuthing. PR would be great.

@galbramc, the pin to pint==0.9 is interesting -- are we setting that in our unit testing scripts? All I see in setup.py is pint >= 0.8.1.

@tblanarik
Copy link
Author

Yeah, I couldn't locate the pin of pint==0.9, and was wondering if it's managed outside of this repo by your Jenkins setup 🤷

@galbramc
Copy link
Contributor

Hey guys! Yes the jenkins scripts were setup to install pint == 0.9. I've removed that now and Jenkins is installing pint-0.20.1. Hopefully you can resolve the issue now!

@tblanarik
Copy link
Author

tblanarik commented Feb 24, 2023

@galbramc - I wonder if that's actually going to break the rest of the tests 😆 , since pint 0.19.1 contains breaking changes.

Since I don't have access to the scripts, I'm wondering if you'd be willing to try pinning all of the Jenkins scripts to 0.18 and rerunning?

In my own fork, I ran your unittests using GitHub Actions, Python 3.9 (which I think your Jenkins is set to) and the following requirements:

pint==0.18
adce==1.3.3.2
cvxopt==1.3.0
numpy==1.23.1
plotly==5.9.0
scipy==1.8.1
matplotlib==3.5.2

and you can see the tests passing on both Ubuntu and Windows, using cvxopt at least (no mosek)

Actions screenshot CleanShot 2023-02-24 at 15 14 24@2x

Alternatively, is it reasonable to make the Jenkins script a part of the repo, to have more 👀 on it?

There must have been some difference between the scripts since only the windows10x64 builds failed.

CleanShot 2023-02-24 at 15 09 31@2x

Finally - perhaps you don't really mind if the unittests are broken and this is just noise to you. I can go away in that case 😆

@galbramc
Copy link
Contributor

What is preventing us from fixing gpkit and/or the unit tests so it works with the latest pint?

@whoburg
Copy link
Collaborator

whoburg commented Feb 25, 2023

well, latest pint is currently broken per hgrecco/pint#1507, right?

@galbramc
Copy link
Contributor

Are we storing any pickles in the repo? Are pickles supposed to be loadable forever or simply be treated as temporary data? Do we just need to recreate the pickled files for the newer pint?

Btw, shouldn't you be getting ready for a ride on a rocket!

@whoburg
Copy link
Collaborator

whoburg commented Feb 25, 2023

ohhhh, good point. yep that's probably a simple fix.

And yes definitely - but I'm also on a campaign to get inbox zero before launch :)

@galbramc
Copy link
Contributor

Inbox zero is far more challenging mission! My girlfriend and I would watch the launch if you had picked a more reasonable time of day :)

@tblanarik
Copy link
Author

The question of loading pickles forever is where I was going to ask you guys what your approach was. If it's that you want GPKit to just always work with the latest releases of packages, awesome 👍. But if you wanted to maintain some kind of backwards compatibility for particular reasons, then this might be a more complicated fix.

Since I'm only just exploring this project for the first time, I don't know a lot about how people are using it / what sort of support for older packages you care to provide 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants