NNPDF
Include evolutionary-keras
This branch contains the same commits as n3fit-nnfit_in_n3fit, but is instead rebased to the current point of master, while we want to preserve n3fit-nnfit_in_n3fit for future reference as this is the branch used to do the test with the NGA and CMA-ES.
Once n3fit has been optimized and TF2.2 released, this branch can be merged.
@RoyStegeman let's wait a few weeks for finishing this, once I finish with the optimization of the n3fit it should be more robust for the implementation. Also, we need to wait until TF 2.2 make it into the conda repos.
@scarlehoff I am planning to bring this PR up again next Wednesday as we're at a point where we can (if we want) include this.
The macOS test fails at build though and, unless building is just slow and it is ended early due to the time limit, the error is not very enlightening. I heard problems with macOS are not uncommon, so do you perhaps understand this error?
Yes, the tensorflow package for mac is set to 2.0 because the conda packagers are not able to make anything beyond 2.0 work in Mac :__ (so conda is not able to get around the dependences so it freezes)
As mentioned in the phone call this can be a good compromise solution where other backends can easily be implemented without permeating to the default conda package.
If people like this solution I'll do it proper (mainly: moving things away from __init__) and solve the conflicts.
@RoyStegeman I've created a "Basic_nga.yml" runcard, but could you put there the entries from one of the actual runcard you use? thanks!
How it works: The idea would be to have the backend-dependent things initialized by default with tensorflow but allow a change of backend.
I like because it is clean and clear and as long as backend are tensorflow-dependent this is perfectly fine. If someone wants to add a pytorch (for instance) backend the amount of work needed to extend this solution to a completely different backend is negligible by comparison.
I adjusted the runcard as you asked.
Previously I hadn't included the optimizer parameters since evolutionary-keras's default parameters are in agreement with the once originally used by NNPDF and wanted to prevent cluttering as much as possible. But now that there's a separate file anyway I added the option to set those as well.
I like your solution, but if I remember correctly I suggested adding an option to the runcard with an if statement somewhere during the meeting which was not well received. Although maybe Zahari will be able to live with this solution, I guess we'll find out next Wednesday.
I think the problem people had was having the dependence and not the runcard option.
Ok, this is ready. @RoyStegeman please have a look and check the docs if you want to add more stuff.
@Zaharid since I'm going the responsible user way I would be more comfortable if you have a look at this
I suppose the main question is why is there so much complicated global state instead of classes? ITSTM the whole complicated _internal_state* code can be wrapped in a more or less small class.
To have the option to select backend you have to have more than 4 lines, of course. The extra backend itself is literally 5 statements: 2 imports, 2 optimizers, 1 class.
ITSTM the whole complicated _internal_state* code can be wrapped in a more or less small class.
If you are talking about set_backend, you could have one class (it must be just one) where its attributes are references to the right backend as I did here: https://github.com/NNPDF/nnpdf/commit/fbb21294f2292c22a6918e22d2d9fc61f1fba8e5#diff-bdb7452b8acc82beceeea55b2a6e68f5R20
but I thought it would be clearer from the point of view of an user who doesn't touch the backend to have an n3fit.backends module where the content modules are different depending on what backend was set.
A possible alternative approach is to have a class and a bunch of top level functions working with an instance of the default class. E.g. https://docs.python.org/3/library/pickle.html#module-pickle Not sure if it makes that much sense, as you could always wrap the "user facing" code in a single function doing everything.
However I'd say a bunch of context dependent imports is bringing in way too many footguns.
However I'd say a bunch of context dependent imports is bringing in way too many footguns.
I think having more than one backend means having context dependent imports.
So, to me the nice thing of these approach is that adding new backends is easy (you only need to add a set_backend function) and from the point of view of the user nothing changes (you have a bunch of modules and classes that you have to import from n3fit.backends).
In this sense I think both setattr and having a class per module are the same and are equally context dependent.
A possible alternative approach is to have a class and a bunch of top level functions working with an instance of the default class. E.g. https://docs.python.org/3/library/pickle.html#module-pickle
I'm reading through the pickle docs but I don't know what you mean / how that would solve the context dependency.
I think having more than one backend means having context dependent imports.
Yes in the sense that some module may or may not be imported, not in the sense that there is some spooky action at a distance that is very difficult to understand by reading the affected module itself. I am not aware of plugin systems that engage in this level of black magic (and would question we want it given just this one instance of plugin).
I wish the code looked more like:
if use_ga_backed:
import evolutionary_bakend # This could be in n3fit or the external module
backend = evolutinary_backend.Backend()
else:
backend = DefaultBackend()
where backbend has all the functionality that varies.
I'm reading through the
pickledocs but I don't know what you mean / how that would solve the context dependency.
There is the class Pickler which implements methods like load or dump and the homonymous top level functions that use some default instantiation.
The way Keras used to do this was with environment variables so that at first instantiation you get the one backend and that's it. I'm happy with this method as well instead of having the option in the runcard or by making vp set the environment if the variable is in the runcard and that's it. What's your opinion on that?
I wish the code looked more like:
if use_ga_backed: import evolutionary_bakend # This could be in n3fit or the external module backend = evolutinary_backend.Backend() else: backend = DefaultBackend()
where backbend has all the functionality that varies.
Then you cannot do from n3fit.backends import operations for instance, which in turns complicates a lot the actual code (in theory the backend is something that nobody needs to touch).
Rebased from master to get the fix for travis.
So in the end what do you guys prefer, a environment variable that selects the backend (and then by default you have tensorflow) or the ability to change the backend on the go?
The advantage of the first is that you don't have context dependent imports more than you already have (not anymore, but until version 2.3 we had a context dependent import since the environment variable KERAS_BACKEND controlled whether keras would use tensorflow or theano, defaulting to theano in debian).
The advantage of the second is that in most cases (both this PR and also some other projects I am working on) the "other backends" are actually extensions on top of TensorFlow so developing wise I like having a function that changes all the imports but I realise why people wouldn't like this.
Edit: I favour now the second scenario since it is much cleaner when building a chain of backends (which I have in a personal project, where backend C extends B which extends tensorflow)
I was CCed but I feel a bit out of my depth here. I think the function which changes the imports seems reasonable enough but I don't really know what the potential hazards are with these things.
Honestly, I don't feel like I have sufficient familiarity with code development to make this choice. At best I can try and weight the arguments you've provided, so it's probably best to aks input from e.g. Stefano if you need a deciding vote...
@RoyStegeman if you don't mind I'll keep rebasing this branch periodically to the current master because I want to keep it up to date.
@scarlehoff do you still have plans for this PR? I think the other backend you were considering was for the quantum computer project, but apparently that didn't end up getting added.
@scarlehoff do you still have plans for this PR? I think the other backend you were considering was for the quantum computer project, but apparently that didn't end up getting added.
it was, in a "secret" repo, but having this (and any other) available in a way that makes people happy (including myself) is more work than it is worth it
It is a working version so if someone is really interested they can run fits with n3fit with a GA.
That's true, although that specific functionality can also be obtained very minimally by inhering the evolutionary keras model instead of the regular keras Model. It doesn't require all this backend machinery. I guess that means we're now at a point where we need to decide to either keep this PR indefinitely or just close it.
The machinery is needed for a more general "choose your backend" scenario. I would leave it open because maybe in a few years time it is useful to some unsuspecting student. If it is closed it will never be found (and we will never get to a 0-issues, 0-PRs scenario)
But I very much doubt it will be ever be used again so if it's closed I'm also ok with it.
Sure, it can be left open, I don't care much either way but since the last thing I heard was that you were planning to add another backend which didn't happen, I just wanted to make sure it was a conscious decision not to close this.