-
Notifications
You must be signed in to change notification settings - Fork 24
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
WARNING:root:Settings file 'settings.py' does not exist, ignoring it #151
Comments
I wonder how in your case Sisyphus imports end up in your RETURNN config. While there can be model code imported in the manager, I have never seen pipeline code being imported into the config for me... So no opinion from my side here. |
Look at that from sisyphus import tk Because in that file itself, I have mixed both the Sisyphus logic and the model + train + recog code. |
That was indeed my first thought as well. my second thought was that
is a bad idea for anyone who wants to see what your experiment did in 3 years time after you have updated your recipes. I would only use the i6_experiments from a CloneGitRepositoryJob with a defined commit, so that it is reproducible. But your recipe design is not the issue here. Going to the question:
Here I agree with you. But are the settings really never used if sisyphus is imported but not the main executable? |
Offtopic mumbling: I consider RETURNN, i6_core, RASR, i6_experiments (at least the parts I developed and/or use in my setups), Sisyphus as stable, so that it should never be a problem also in 3 years to run it (or if this does not work, I consider it a bug which should be fixed). I don't want to use CloneGitRepositoryJob for any of those. I find it kind of arbitrary that you use i6_core, Sisyphus as is (not part of the hash), also Linux, Apptainer, the node you run on, whatever other things not part of the hash, but the others (or whatever selection you have) are part of the hash. I also get the feeling we are really overthinking the whole hashing part. In the logs, ideally you should also see a date, and maybe the commit/version of all relevant software, so even if sth breaks, you can still reproduce it. Or even if you don't have that, you can still probably get to a working version back in time. This is anyway a case which is rarely if ever relevant. Anyway, back to topic:
In this case, I don't use anything at all from Sisyphus. All Sisyphus related calls are in separate functions which I don't call inside RETURNN. |
I would consider it best practices to separate these two, to avoid unnecessary imports. Beside that you are right that it should be possible to include Sisyphus without getting a warning. How about having a global sisyphus config file e.g. $HOME/.sisyphus which is always loaded first. This would allow to set stuff like the grid engine globally and also allows to define if there should be a warning if no local config is found. |
What about my initial suggestion?
|
I'm a little reserved about that, if you import sisyphus to run a workflow inside a script it would also hide the warnings. |
With our new i6 setups, a RETURNN config file often directly imports model definitions or other things from the i6_experiments repository. That can look like this:
As the i6_experiments code is mixed with Sisyphus-level logic and with model definitions which are imported here, this will thus end up with an
import sisyphus
inside of RETURNN. That's why you see bothsys.path.insert
there.But
sisyphus
is just imported here but otherwise not really used. However, it still leads to this warning:I think we should not print this warning, unless the settings are actually used.
The way this is designed in Sisyphus makes this tricky, though.
However, a simple thing we can do is maybe checking
sys.modules['__main__'].__file__
if that is actually some Sisyphus executable or not (like RETURNN) and in case it is not from Sisyphus, do not print the warning.Or what do you think?
Or maybe you say this is really intended, even at just
import sisyphus
, wherever you do that?One workaround on RETURNN side (or wherever else you might have an
import sisyphus
):Add sth like this before the import:
So we could add this to the RETURNN config. But it's also somewhat ugly.
The text was updated successfully, but these errors were encountered: