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

Use dill for pickling if available. #139

Merged
merged 3 commits into from Jul 15, 2013
Merged

Use dill for pickling if available. #139

merged 3 commits into from Jul 15, 2013

Conversation

twiecki
Copy link
Contributor

@twiecki twiecki commented Jul 3, 2013

As described here:
https://github.com/jaberg/hyperopt/issues/131

Passing a non-lambda function to fmin() with the mongo backend doesn't work as cPickle can't pickle functions. Dill, however (https://github.com/uqfoundation/dill) provides a pickle-like interface that makes this possible.

To remain backwards compatibility if dill is not installed I implemented a fall-back mechanism.

@jaberg
Copy link
Contributor

jaberg commented Jul 4, 2013

@twiecki I would be happy to merge this PR, but I wanted to ask you to do one more thing if you don't mind: could you track down the cPickle call that fails if you need dill, and add some sort of hint in the error message to point out that installing dill would help?

The use case I'm imagining is

  1. user downloads hyperopt
  2. tries to use it
  3. sees cPickle fail
  4. gets frustrated, doesn't know about dill.

Can you remember where to add a hint so that the next user sees a helpful hint instead of hitting (4) ?

@twiecki
Copy link
Contributor Author

twiecki commented Jul 4, 2013

@jaberg Totally agree, I'll make the necessary change.

@twiecki
Copy link
Contributor Author

twiecki commented Jul 11, 2013

@jaberg OK, let me know what you think. I didn't shadow the exception but rather display a log message.

@jaberg
Copy link
Contributor

jaberg commented Jul 11, 2013

@twiecki This looks good, but I'm thinking -- doesn't pickling usually fail on the dumps call, rather than the loads ? Did you put the log message in the right place?

@twiecki
Copy link
Contributor Author

twiecki commented Jul 11, 2013

@jaberg So actually you can pickle a function call just fine but as it's unpickled it will try to resolve it but can not find the referenced function in the namespace. So we certainly need it there. I can add another check to dumps though.

@twiecki
Copy link
Contributor Author

twiecki commented Jul 15, 2013

@jaberg OK, added a try/except around dumps too.

@jaberg
Copy link
Contributor

jaberg commented Jul 15, 2013

@twiecki Thank you so much!

jaberg added a commit that referenced this pull request Jul 15, 2013
Use dill for pickling if available.
@jaberg jaberg merged commit c477d89 into hyperopt:master Jul 15, 2013
@jaberg
Copy link
Contributor

jaberg commented Jul 15, 2013

@twiecki This branch had except ImporError instead of ImportError so it wouldn't even import hyperopt. You'll probably want to update to master!

@twiecki
Copy link
Contributor Author

twiecki commented Jul 15, 2013

Oops, sorry about that! unittests apparently didn't choke on that.

@jaberg
Copy link
Contributor

jaberg commented Jul 15, 2013

Interesting! Unit tests were meant to run with or without mongo, using
import error to detect a missing mongo... apparently that can go
spectacularly wrong.

On Mon, Jul 15, 2013 at 3:40 PM, Thomas Wiecki notifications@github.comwrote:

Oops, sorry about that! unittests apparently didn't choke on that.


Reply to this email directly or view it on GitHubhttps://github.com/jaberg/hyperopt/pull/139#issuecomment-20998000
.

@jaberg
Copy link
Contributor

jaberg commented Jul 15, 2013

Hmm, no the logic in the test_mongoexp look OK. I'm guessing what happened
is that a syntax error makes nosetests ignore the file, but I'm not going
to follow up on that now. Things seem in order again.

On Mon, Jul 15, 2013 at 3:42 PM, James Bergstra james.bergstra@gmail.comwrote:

Interesting! Unit tests were meant to run with or without mongo, using
import error to detect a missing mongo... apparently that can go
spectacularly wrong.

On Mon, Jul 15, 2013 at 3:40 PM, Thomas Wiecki notifications@github.comwrote:

Oops, sorry about that! unittests apparently didn't choke on that.


Reply to this email directly or view it on GitHubhttps://github.com/jaberg/hyperopt/pull/139#issuecomment-20998000
.

@twiecki
Copy link
Contributor Author

twiecki commented Jul 15, 2013

OK, sounds good.

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

Successfully merging this pull request may close these issues.

None yet

2 participants