ReturnnTrainingJob info, more robust LR log loading #549
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In my case, I had some np.float64(...) entries in it (for the effective LR). Not sure how I got that (maybe due to new PyTorch/Numpy), and I already pushed some RETURNN change to avoid this in the future, but I think we can anyway just handle that here as well.
albertz
requested review from
NeoLegends,
JackTemaki,
michelwi and
Atticus1806
michelwi
approved these changes
Oct 17, 2024
| @@ -287,6 +287,8 @@ def _get_run_cmd(self): | |||
| return run_cmd | |||
|
|
|||
| def info(self): | |||
| import numpy as np | |||
There was a problem hiding this comment.
what was the reasoning behind doing a local import vs module level?
There was a problem hiding this comment.
- I like to keep things local to see where they are used. That's a thing you should do in general, for all kinds of things, about the structure of code (e.g. local variables, etc.).
- It's not good practice to put too much things into the global namespace.
- It could increase the module load time when
numpywas not imported yet.
There was a problem hiding this comment.
Note, the only some (valid) arguments to put the import at module level:
- You see all the dependencies of the module more easily. (But I'm not sure this is so important here.)
- Convention
- Save some lines of code when the import is used often. (This is very valid, when most of the code in the module is actually depending on it, which is often the case for
tensorflowortorch.)
There was a problem hiding this comment.
It comes very surprising to me to break with the convention now, but I do not care too much.
the only
Definitely not the only in general, maybe the only one here. Once you use it more often you save lines of code, definitely a benefit.
JackTemaki
approved these changes
Oct 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In my case, I had some np.float64(...) entries in it (for the effective LR).
Not sure how I got that (maybe due to new PyTorch/Numpy), and I already pushed some RETURNN change to avoid this in the future, but I think we can anyway just handle that here as well.