-
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
Clarification: canonical way to define custom global settings #82
Comments
sisyphus.global_settings was not meant to be used inside recipes. It's there to define sisyphus specific settings, e.g. timeouts, which engines to use or which environment should be set during job execution. Workflow related things should be defined inside the recipes or config directory. If you want to define all external paths in one place you could create a file either inside the recipe folder or, especially if you don't want to cleanly share it across multiple recipes, inside your config folder. This file can then be imported in all relevant places. It would be possible to define the used path inside a tk.Path object there, or use the output of a job which sets everything up. |
Maybe you have a more specific example how it look like? Don't you also have sth similar in your recipes? In i6_core, there is for example RETURNN_PYTHON_EXE as mentioned, and some others. So there needs to be some well defined place where these are specified. Basically very much like global_settings. Maybe sth like i6_core_global_settings? Does it really make sense to have that file inside config, while global_settings is in the root? This seems a bit inconsistent to me. Also, maybe it should use a very similar mechanism like global_settings, that there are some defaults (defined in i6_core) and then it additionally loads a user provided file which potentially overwrites these defaults? How do you handle that in your recipes? |
Why doesn't it make sense to have it inside the config or recipe folder? It should be doing something totally different to If anything We currently have these path inside our recipes, but our recipes are not split across multiple repositories. So in your case I would do something like:
|
I thought the config folder is specifically to define the job graph to compute, and/or the final outputs to compute and nothing else. But then my understanding of that was simply wrong. Ok then. So, in i6_core recipes, we would have some file like |
The idea is to keep everything that is mainly static inside the recipes like job definitions or static workflows. The parameters to these more static stuff should than be placed inside the config directory. I must admit the line between these two is fairly blurry in practice, especially for the workflows... Please don't use global_settings or any of it's supporting functions, it's not meant to be used outside of sisyphus internal functions and might change at some point. I probably should have called it Overwriting stuff automatically if config.default is available can be done simply like this:
This way you can have some default values in
|
I assume Also, instead of:
sth like |
You can name it what ever you like You are right, |
But default does not really make sense in this example, right? The user might want to mix different recipes. So the name for this config file should somehow be reserved for i6_core.
But this would still have the same problem and hide other |
I don't know if you like want to use an extra settings file per recipe folder or use a share one for all recipes. A shared one is simpler, separated files is more flexible, but this should be discussed over at i6_core. Feel free to write a specific function for it if you like, but using Why don't you just check if the exception was raised for the right file? try:
from config.i6_core_settings import *
except ModuleNotFoundError as e:
if e.name != 'config.i6_core_settings':
raise |
Sure, I just meant to do it conceptually in the same way.
Is this ( But why do you do it differently in |
The code is mainly historically grown and checking some special cases. Also given that |
I think @albertz is profoundly missing the point. i6_core does not need a settings file. The sisyphus recipes contain:
The configs contain:
As an example let's look at the Instead we should simply use a different mechanism to provide a default value to
Then, if anyone needs or wants a different version of returnn, they can override this default by either the output of a different |
Isn't this a contradiction, or just rephrasing "settings file" as "default software" file? Or do you say that it should not be allowed to overwrite the default? But this is maybe debatable. E.g. why should you be able to overwrite the default env (via Sis global settings) but not this setting? The env probably has a much bigger effect on differences in the results than this setting (different RETURNN versions should never produce different results; but for example different TensorFlow versions, or different FFmpeg versions, or different CUDA versions, or whatever likely will produce different results). I don't think the default you proposed is a good one. I would want to have a specific custom path (e.g. (Your When we now discuss this more specifically about i6_core, and more specifically about Btw, other example are also |
@michelwi wrote in rwth-i6/i6_core#32:
I want to clarify: Are we abusing this? If yes, how should we do it instead? I.e. where/how should we define some global settings (which we intentionally do not want as arguments to jobs)?
Let's not argue too much about
RETURNN_PYTHON_EXE
specifically here. This is just an example. This is actually an example where some people prefer it one way (having it as an explicit job arg) and others the other way (use global default, which is just a recent version), so it stays an argument where the defaultNone
would fallback to the custom global setting. Maybe we will often have it like this that both variants make sense. Maybe there are also other cases where we really want to have custom global settings which are never an argument.My take here is that we can simply extend the scope of
sisyphus.global_settings
to also support such use cases. I don't think we need another separate mechanism for this. So basically no change needed then, just maybe some clarification in the doc that this is a valid use case.The text was updated successfully, but these errors were encountered: