Skip to content

Conversation

@bqpd
Copy link
Contributor

@bqpd bqpd commented Nov 30, 2016

Closes #963
Addresses debugging flow of #959

@bqpd
Copy link
Contributor Author

bqpd commented Nov 30, 2016

@whoburg, ready for review.

@bqpd
Copy link
Contributor Author

bqpd commented Dec 1, 2016

Failing only because of MOSEK 8. @whoburg this is ready for review.

@galbramc
Copy link
Contributor

galbramc commented Dec 1, 2016

Still not the right answer. You now find the 32 bit mosek_cli on WIndows 7, but gpkit will fail if it tries to use it. You don't see a failure at the moment because I'm forcing only the cvxopt solver.

@whoburg
Copy link
Collaborator

whoburg commented Dec 2, 2016

@bqpd, waiting to hear a response to @galbramc's concern above before reviewing.

Also, what is the status of Mosek 8 support?

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

re: MOSEK 8, not yet begun.
re: finding 32bit MOSEK, I'm a little confused why a 32bit MOSEK is in the 64bit Program Files folder on a 64bit operating system... For now, my priority is fixing observed problems from users; making sure that MOSEK 32bit has a friendly error is next, along with testing the other Windows install bugs.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

Oops, I had it backwards. Windows 7 has a 32 bit version of python using pythonxy, but mosek is 64bit. That combination does not work. I never upgraded windows 7 to us anaconda after we realized that you could install cvxopt with anaconda.

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

oh hmm, so it's py32 sys.exec not working with mosek64. updated the issue accordingly...

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

Yeah I don't know how the mosek_cli works, but from what I recall the mixture of bits don't play nice with each other. Let me turn of the forcing of cvxopt and see what happens.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

test this please

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

Not the error that I recalled, but this is what you get now:

https://acdl.mit.edu/csi/job/gpkit_PullRequest_Unit/buildnode=windows7x64,optimizer=cvxopt/1744/console

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

Hmm! It looks like it successfully ran the mosek_cli command? But did not like mosek, and so did not set the path...that's a bug. I'll push a fix up.

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

Looks like it's using mosek_cli just fine! Just the MOSEK 8 bugs.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016 via email

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

Test this please

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

oops, @galbramc, I think it's still finding MOSEK 8, but for mosek_cli only?

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

@whoburg the code is ready for review, though tests are pending.

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

retest this please

Copy link
Collaborator

@whoburg whoburg left a comment

Choose a reason for hiding this comment

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

would like to see the tests pass before we merge. And the github home page currently has a red light for unit tests -- I'd like to fix that before merging this.

settings["mosek_bin_dir"])
if "mosek_bin_dir" in settings:
os.environ['PATH'] = (os.environ['PATH'] + ':%s' %
settings["mosek_bin_dir"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

":".join([os.environ['PATH'], settings["mosek_bin_dir"]])

seems cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still doesn't fit on one line, sadly.


def look(self):
"Attempts to import mskexpopt."
"Attempts to import cvxopt."
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

try:
import gpkit
log("# Moving to the directory from which GPkit was imported.")
os.chdir(gpkit.__path__[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, does this have the potential to cause problems if there are multiple installs of GPkit and the user wants to run the build script to overwrite the currently-installed one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, shouldn't have said multiple installs -- rather, envisioning a case where a user has a system-wide install (in Python site-packages) but wants to run the build script from e.g. a clone of the git repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the install step will remove the already installed one before building, and this passes tests and helps with #959 so I figured it was worth a try.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok yeah, sounds like the removal step addresses the concern.

_repr_latex_ = _repr_latex_

def str_without(self, excluded=()):
def str_without(self, excluded=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

fine, but seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh, yeah.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

Hold on, I'm updating the testing script.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

test this please

@whoburg
Copy link
Collaborator

whoburg commented Dec 2, 2016

code is approved but I don't want us to merge until two conditions are both true:

  1. all three jobs on the github home page are showing green
  2. all tests pass on this PR

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

retest this please

@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

(@galbramc, on unit tests but not dependencies still finding mosek8 on win10)

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

I just reinstalled mosek 7.

@galbramc
Copy link
Contributor

galbramc commented Dec 2, 2016

test this please

@whoburg
Copy link
Collaborator

whoburg commented Dec 2, 2016

@bqpd, looks good to merge.

@bqpd bqpd merged commit d1c9591 into master Dec 2, 2016
@bqpd bqpd deleted the fix963 branch December 2, 2016 03:55
@bqpd
Copy link
Contributor Author

bqpd commented Dec 2, 2016

It is done!

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.

4 participants