-
Notifications
You must be signed in to change notification settings - Fork 6
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
Restart hyperopt #1824
Restart hyperopt #1824
Conversation
Thanks @Cmurilochem , do you understand why the test is failing in ubuntu? |
Hi @RoyStegeman, I added a new test that compares the results of one restart and direct hyperopt runs. This then checks for files and depend on paths and from where you run Note: The test (as it is) is not expected to pass entirely since (among other asserts) it requires that the final json ['results'] dictionaries of both runs should match here. This relates to my comments above regarding the differences in the hyper losses for different folds. @goord gave me a nice idea on how to investigate this issue; further details to come. |
That's actually a good question. Since you added the |
Thanks a lot @Cmurilochem for these works! Regarding the issue you are facing now, are you sure that the other seeds (tr/vl, MC replicas) aren't also different? In any case, I think that there should be some (simple) ways to trick the random generators that it is starting from a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments but not a complete review
Thanks @Radonirinaunimi. This is something that I will need to check. Thanks for pointing this out. At least, the provisory change to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And these are my comments for now. Thanks for the work so far!
I suspect that this might be due to the fact that the seeds for the initial weights for each k-fold in difference runs are inherently different (see below).
I suspect so as well - I noticed you froze the seed for the folds but probably not the tensorflow/numpy/python seeds. If the disagreement is due to the effect of random seeds it's of course not a problem for a real run of the hyperoptimization, and for your tests you already freeze them by setting debug: true
so that should also be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Cmurilochem, here is a quick review. These are mainly formatting/styling and asking clarifications for various points.
In relation to the replica dependence. In the multireplica case I suspect that self._nn_seeds (= nnseeds argument in ModelTrainer) is already a list of seeds for each replica. I am not quite certain but, please, correct if I am wrong.
Yes, that is correct! The status of master right now is that it can have different seeds per replica for MCseed and NNseed during a multireplica fit, the only seed that is always the same is the TrVlseed
Hi @Radonirinaunimi. I have corrected for all @RoyStegeman's suggestions and now will proceed to yours. Thanks for you time in reviewing and for your excellent suggestions. |
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
…irectories of tmp_path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
HYPEROPT_SEED
is going to remain fixed for now?
Hi @RoyStegeman that is what we initially thought and implemented so far. But If you think it would be better, I could try to add a new entry into the runcard so that (like other seeds) the user could have control over it. Please, just let me know what do you think ? |
Don't worry I'm fine either way, was just making sure it wasn't something you had forgotten about as I had understood you planned to take it from a runcard seed. |
Great! Maybe we could add this feature in the near future as long as we feel the need to do so. I will keep this in mind. |
Thanks a lot @Cmurilochem! I guess the only minor thing missing in order to merge this PR is a small note in the documentation (at the end of https://docs.nnpdf.science/n3fit/hyperopt.html?highlight=hyperopt) describing how one can restart hyperopt. |
Hi @Radonirinaunimi. I could add a note after Changing the hyperoptimization target and let you know after the commit has been done. |
Ah good point! It needs to be documented of course. Completely forgot about that 😅 |
Thanks @Radonirinaunimi and @Radonirinaunimi. Documentation added! Please, feel free to suggest any possible changes and/or additions. |
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Hi @RoyStegeman, @Radonirinaunimi and @scarlehoff. Please, let me know whether I could merge this PR after the approval of Roy and Tanjona. Thanks you all for your very valuable suggestions. |
Yes, please merge this |
This PR addresses the issue of restarting an hyperoptimization with the Hyperopt library as discussed in #1800.
Comments on the initial changes made
1.
hyper_optimization/filetrials.py
FileTrials
class I have added thefrom_pkl
andto_pkl
methods. The last one is a@classmethod
that is useful to create instances of the class whentries.pkl
file is available from a previous run.The
to_pkl
method saves the current state ofFileTrials
to a pickle file, although this is currently being indeed done for every trial inhyperopt.fmin
directly via thetrials_save_file
argument.self.pkl_file
which will be responsible for generating atries.pkl
file in the same directory as thetries.json
self._rstate
is also added that will store the lastnumpy.random.Generator
of the hyperopt algorithm and will be passed asrstate
in thehyperopt.fmin
function so that we warrant thatby restarting we do so with the same history as if we were doing a direct calculation. The initial fixed seed in
trials.rstate = np.random.default_rng(42)
here can still be relaxed and provided as input later.2.
hyper_optimization/hyper_scan.py
HyperScanner
, namedself.restart_hyperopt
which is settrue
in case of the--continue
option inn3fit
command line (details to be discussed below).hyper_scan_wrapper
to allow it to check ifhyperscanner.restart_hyperopt
istrue
. If so, it will generate an initialFileTrial
instance (trials
) fromtries.pkl
, which contains by built-in the history of the previous hyperopt and also thetrials.rstate
attribute with the previous numpy random generator.3.
scripts/n3fit_exec.py
This is perhaps the most fragile of the changes and where I would need help to adapt it properly.
N3FitApp
I added a new parser--continue
that will be the keyword to hyperopt restarts.run
method I add a newself.environment.restart = self.args["continue"]
attribute.HyperScanner
later is to use it in connection withproduce_hyperscanner
. If this istrue
, I then updatehyperscan_config
withhyperscan_config.update({'restart': 'true'})
and this will later be part of theHyperScanner
'ssampling_dict
argument.Questions and requested feedback
scripts/n3fit_exec.py
file to allow for--continue
are not optimal. Maybe a more experienced developer could suggest a more convenient way to do so.differences in the obtained final losses for different
k
folds. This might be due to the fact that the seeds for the initialweights
for each k-fold in difference runs are inherently different (see below).For example, I have done a test in which I make a simple hyperoptimization with 2 trials, and then restart it to make another 2 trials (4 in total). Then I run another experiment and calculate (with the same runcard) 4 trials directly and compare the results.
By looking at the above results, we can see that Restart 2/3 have the same hyperparameters as Direct 2/3, with the 2 folds having different losses however. Maybe the 1st fold can still keep up with the losses but not the second fold.
With the help of @goord and @APJansen, I investigated this issue and have printed the generated random integers passed as
seeds
to generate the PDF models for each fold inMoldelTrainer.hyperparametrizable()
; see here. They are shown in the Table below:As foreseen, it is clear from the table that the
seed
s are different for the second fold every time we run a new calculation, despite the fact that the runs start with the same hyperparameters. This clearly reflects in the different losses shown above. I suspect that if we want to make hyperoptruns completely reproducible we could think of alternatives to
to initialise the seeds.
Solution to the random integer issue described above
4.
model_trainer.py
To ensure that these
seeds
are generated in reproducible way, @RoyStegeman helped me to devise a new form that changes the way they are generated by defining:With all the above modifications, I have repeated my previous 4 trial experiment. The results are shown below for both restart and direct runs:
As seen, we are now able to ensure that both the hyperparameter space and the initial
weights
for each k-fold are reproducible when restarting.Note
As can be seen from the above (last) table, because the seeds to generate the random integers for each k-fold are now derived from the fixed value
self._nn_seeds
here, the generated random integers will always be the same in every trial; see #1824 (comment). This is an important aspect to keep in mind.