Skip to content

WIP: Add mimir logger#1013

Open
dmitriy-serdyuk wants to merge 47 commits intomila-iqia:masterfrom
dmitriy-serdyuk:mimir-backend
Open

WIP: Add mimir logger#1013
dmitriy-serdyuk wants to merge 47 commits intomila-iqia:masterfrom
dmitriy-serdyuk:mimir-backend

Conversation

@dmitriy-serdyuk
Copy link
Contributor

First working draft of a mimir logger.

  • Figure out if (un)picking works. Currently I just save the kwargs during picking and recreate logger during unpicking.
  • Save 1d numpy arrays in a more readable format.
  • Add some tests.
  • Add documentation.

@rizar , @bartvm

self.iteration_status = {}

def __setstate__(self, state):
logger = Logger(**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

kwargs should be self.all_kwargs I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, it should be state.

@dmitriy-serdyuk
Copy link
Contributor Author

@bartvm , seems the log writes only the last two lines to the file with an option max_len=2. I supposed, that this argument concerns only the information stored in memory.

@bartvm
Copy link
Member

bartvm commented Mar 2, 2016

That doesn't sound right, max_len should only affect the entries kept in memory. Did you try it without your wrapper?

@dmitriy-serdyuk
Copy link
Contributor Author

I fixed that problem, it was my bug.

I checked locally pickling/unpickling and it works fine, but Travis doesn't like it...

@bartvm
Copy link
Member

bartvm commented Mar 2, 2016

I think the problem is that the log doesn't get closed. Until the log gets closed, gzlog maintains a lock file (log.jsonl.lock), and if you try to open a second log with the same filename it will wait for up to 5 minutes to obtain the lock.

If you don't call close explicitly the lock will only be released when close is called by io.RawIOBase's __del__ method (when it gets garbage collected), but if multiple tests are run in a single Python interpreter, I guess it's guesswork when this actually happens.

@dmitriy-serdyuk
Copy link
Contributor Author

My implementation doesn't allow to access log entries more than 3 iterations before. It seems for me as a reasonable solution for many tasks with a large log.

But it means, that I cannot run all the tests (some of them are trying to check that the whole log is fine). I can see two solutions:

  • Disable these tests for Mírmir log.
  • Allow the access loading the whole log into memory as a separate Logger object.

Neither seems nice to me, what do you prefer, guys?

@bartvm
Copy link
Member

bartvm commented Mar 3, 2016

An alternative solution would be to make maxlen a configuration setting with a default of 3. For those tests you can then temporarily change it from 3 to None so that the entire log gets kept?

@dmitriy-serdyuk
Copy link
Contributor Author

Seems, that no:

In [4]: a = numpy.array(1)

In [5]: isinstance(a, numpy.generic)
Out[5]: False

@bartvm
Copy link
Member

bartvm commented Mar 3, 2016

Right, that's not a scalar but a 0D array. I can add that case (if a.ndim == 0) to Mimir I guess.

Just wondering, where in Blocks are 0D arrays created? Reduction operations like sum and mean should generally return scalars instead of 0D arrays.

@bartvm
Copy link
Member

bartvm commented Mar 3, 2016

Also, in general I'm not sure whether the numpy.asscalar call is a good idea. The problem is that this works on any array with a.size == 1. So if you have log entries with 1D arrays of variable length (e.g. np.array([1]) and np.array([2, 3])) some will be turned into Python scalars and others won't be, which could be messy when deserializing. By checking if ndim == 0 you'd avoid this.

@dmitriy-serdyuk
Copy link
Contributor Author

As I can see, theano always returns 0d arrays even if the function is compiled with tensor.scalar type.

@bartvm
Copy link
Member

bartvm commented Mar 3, 2016

Looks like it. Seems like an inconsistency between Theano and NumPy:

In [1]: from theano import tensor

In [2]: import numpy as np

In [3]: x = tensor.vector('x')

In [4]: x.mean().eval({x: np.random.rand(10)})
Out[4]: array(0.40431822667528144)

In [5]: np.random.rand(10).mean()
Out[5]: 0.43414875634541017

@bartvm
Copy link
Member

bartvm commented Mar 3, 2016

bartvm/mimir@cd8167f

.travis.yml Outdated
- python: 3.4
env: TESTS=blocks FLOATX=float64
- python: 2.7
env: TESTS=blocks FLOATX=float32 DB=mimir
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that it is worth to run all these additional tests every time. We can at least skip Python 2.7 tests for Blocks and only do them for Blocks-examples.

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 agree, that's too much. I'll remove some tests after I'll be sure that everything works.

@bartvm bartvm mentioned this pull request Mar 3, 2016
@dmitriy-serdyuk
Copy link
Contributor Author

I restarted the master branch build and it also fails, Travis has some problem again, since there's no g++ somehow.

@bartvm bartvm mentioned this pull request Mar 4, 2016
self.status = {}
TrainingLogBase.__init__(self)
kwargs.setdefault("maxlen", 21)
kwargs.setdefault("filename", filename)
Copy link
Member

Choose a reason for hiding this comment

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

You don' have to do this, Python will complain if a user passes filename as a positional as well as a keyword argument.

I think it would be cleaner to just use __init__(self, filename='log.jsonl.gz', maxlen=21, formatter=None, **kwargs) though and then call PicklableLogger(filename=filename, maxlen=maxlen, formater=formatter, **kwargs). Better for default arguments to be in the signature than hidden in the code.

@bartvm
Copy link
Member

bartvm commented Mar 4, 2016

@dmitriy-serdyuk Please rebase on master and give Travis another try!

@dmitriy-serdyuk dmitriy-serdyuk force-pushed the mimir-backend branch 3 times, most recently from cfe9c3e to ad4eced Compare March 8, 2016 22:21
@bartvm
Copy link
Member

bartvm commented May 10, 2016

@dmitriy-serdyuk Did this ever end up working?

@dmitriy-serdyuk
Copy link
Contributor Author

Not yet =)

I promise that I'll find time to finish it. I'm starting to think that it's much easier to change the logger interface than trying to mimic the default dict.

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.

4 participants