Import file design

[albertz]

The idea came up a couple of times that it would be nice to be able to import external files into your config.

The external files would be Python files and could provide building blocks (e.g. a subnetwork dict, or some generic function like make_trafo_block, or also make_librispeech_dataset, or whatever). It should be very easy to share our work among each other.
(Some parts in RETURNN maybe would need to be extended, to make that easier. But that would be a separate discussion. And I think even right now, a lot is already possible that way.)

The import would be from Git repos. I imagine a very simplified version of Go imports. You would specify sth like import("github.com/rwth-i6/returnn-experiments", "2019-librispeech-system/attention/base2.bs18k.config").
(Maybe see related Go code: Git fetch etc.)
(We also might allow imports by local filename, like import("", "/u/zeyer/....") during development, but this should really be temporarily. And maybe also always expect this must be a Git repo.)

The import should enforce explicit versioning, so that an update can never break anything. The user would explicitly specify what version he wants. E.g. like import("github.com/rwth-i6/returnn-experiments", "2019-librispeech-system/attention/base2.bs18k.config", "dc363cb"). (And that maybe also for local files, if we allow that.)
The Git ref should really be a hash. We should not allow "HEAD" or "master" or so. We should even not allow Git tags. It should not be possible to break some configs by an update in the repo. The user would be responsible for updates.
However, we might extend it by meta information, such that the code becomes more readable. E.g. extend the ref by a date, so you could write "2019-04-04--dc363cb` or so. Maybe we should even enforce that.

There should be a local cache for the Git repos of the imports. See the related Go get logic. This can be shallow and thus would not take much space. However, we should maybe clarify how this would be organized. Should this be in /var/tmp like the native-op compile cache, or be more permanent in the home dir (like Go get).

So, let's discuss some of the details, or the generic API here. Maybe other important ideas or aspects we should consider before we design and implement this.

[albertz]

I'm still thinking about the common development pipeline. I think during development, you probably don't want to use any commit, but just the current file tree. Just for testing whether your logic works. At least I cannot really think about a better way. Do you have any suggestions about this?

I'm also thinking a bit more where (and how) we would store the cloned repos / checkouts. Go by default stores them in ~/go/pkg/mod. See Go Modules References. And then the checked out dirs include the web path and version. Examples:

go/pkg/mod/github.com/golang/freetype@v0.0.0-20170609003504-e2365dfdc4a0
go/pkg/mod/github.com/golang/mock@v1.1.1
go/pkg/mod/github.com/srwiley/oksvg@v0.0.0-20200311192757-870daf9aa564
go/pkg/mod/github.com/gabriel-vasile/mimetype@v1.1.2

I already mentioned, I don't like version numbers too much, because they don't really tell you anything (except if you already know the package). I think a date + commit ref is more helpful. (Btw, Go uses const minHashDigits = 7. And it seems 12 digits for the pseudo version with date-commit.) Go has both. The problem is, once you introduced version numbers in some package, you cannot go back to the date+commit pseudo version.

(Btw, technical detail: You would probably not store all Git objects multiple times for any such repo@v... checkout, but somehow share them, maybe just by symlinking the .git dir, or maybe have a common repo@bare dir or so.)

Go directly uses the home directory, there is a go subdirectory. Not sure if people find this annoying, or if this is ok. Python stores (installed) packages in ~/.local/share (but maybe not exactly comparable). Not sure what other schemes are there. We can anyway just decide what we prefer, what would be most convenient. The canonical path would be ~/returnn, and then maybe we would have packages in ~/returnn/pkg (no need for ~/returnn/pkg/mod I think...).

So, back to the development workflow: Maybe you can also have one checkout not pinned to any version? Sth like ~/returnn/pkg/...repo@dev. In there, you can make changes. For the import, contrary what I said before, we would allow not specifying a version. (I still don't really like this. But I don't really see another good option. I also had the idea to make only allow this if some env var like RETURNN_DEVELOPMENT=1 is set or so. But this might just be annoying as well and then people will just have it in their ~/.profile and this doesn't help anyone.) I probably would also print some warning, like:

WARNING: Import ... without explicit version, using development version from <date>-<commit> (DIRTY)
Apply this patch to make it permanent:
- import(...)
+ import(..., "<date>-<commit>")

Or sth like this.

(Btw, technical detail: import is a reserved keyword, so we would need to use another name. Maybe import_mod? Or import_? import_remote? import_extern? extern_import? How would we anyway call this whole mechanism inside RETURNN? Where would we implement this in RETURNN? I suggest sth like returnn/util/import_/ but not sure. This name should be consistent with the main import function name.)

[albertz]

Maybe just leave @dev away and use the name as-is...

[albertz]

Btw, one motivation of this concept is that RETURNN really should be more like the stable base core framework, having (mostly) only stable features, and most experimental things would go into such external repos, such that you can easily play around with them, modify them, etc. That should be all modular that mixing it in whatever way is not problematic.

In case you think for some aspects this is currently not possible, or difficult, or not convenient enough (e.g. having some custom optimizer, having more custom training loop, custom learning rate, custom dataset, whatever really), then consider this a bug, and open an issue about it. We should make RETURNN in a way that really everything should be flexible.

[albertz]

One place where we could share custom recipes is the returnn-experiments repo. Maybe by topic, sth like returnn-experiments/performer/base.py and people could have their custom variants in other files like .../custom_albert.py or whatever. Maybe we should open a separate discussion thread just about this, e.g. how to properly organize this.

You might also want to keep some stuff private initially. In that case, you could use GitHub private repos, or the i6 internal Git server (Phabricator), or some other service. This should be no problem.

[albertz]

Note, I created a common directory (Python package) now in the returnn-experiments repo, and plan to use this for a bunch of recipes. It should probably have some default models, datasets etc (e.g. also a "default Transformer").

But the idea is that this is kept simple, and when you need some customization of the Transformer, you would not add options to the default Trafo, but instead copy it, and have trafo_xl.py then, or whatever.

I want to avoid having one single Trafo with hundreds of options (where 95% are not used) which is extremely difficult to read and understand as well. The code should be simple and clean.

I also want to avoid deep nested class hierarchies. You will e.g. find them in Fairseq for some model definitions. I think it also makes it quite difficult to really understand, and also to modify further.

Those are all not really strict rules, but rather guidelines. And you can always have your custom trafo-v2.py or trafo-patrick.py in there or whatever, if you don't like the default. Or maybe also have a completely separate package (or repo). Although I think it's nice if it is all in one place.

[albertz]

Another thing: The current suggestion is for single Python files (mods). But maybe we should also allow to import Python dirs (packages)?

[patrick-wilken]

Having a repo with higher-level building blocks - mostly parts of network definitions I guess - would be nice, I agree. How would this be maintained? Would this be a place to share code for everyone or more like a "peer-reviewed" set of extensions to core RETURNN?

I wasn't familiar with go imports until now, seems to be a nice idea. But the more obvious alternative would just be that the user clones (or pip installs) the shared repo/module. Which wouldn't require any new import mechanism. So the advantage of having such imports is that version control is built in, right?

We should even not allow Git tags.

Because we don't rely on people not force pushing tags? We usually work with tags, so this would have been the natural way for us. But anyway, in a shared repo would anyone add the tags he needs? This might become messy.

[albertz]

Having a repo with higher-level building blocks - mostly parts of network definitions I guess - would be nice, I agree. How would this be maintained? Would this be a place to share code for everyone or more like a "peer-reviewed" set of extensions to core RETURNN?

So far, the discussion here was mostly about the technical base, and not at all about how to manage such a repo (or maybe multiple repos). Although this is definitely sth we maybe should discuss. I'm not sure there is really a good optimal way. So that's why such a technical base should be flexible to allow for multiple possible variants. Maybe we first try out a simple variant in the beginning (using a simple repo, maybe just directly to returnn-experiments) and later try out other variants.

In there, maybe there would be some standard transformer.py, but then when people try out things, they might have their custom version, like trafo_albert_v1.py or whatever, and this would be without peer-review. The versioning would ensure that you anyway would never break sth.

I wasn't familiar with go imports until now, seems to be a nice idea. But the more obvious alternative would just be that the user clones (or pip installs) the shared repo/module. Which wouldn't require any new import mechanism. So the advantage of having such imports is that version control is built in, right?

But how would you load that in the config? Unless your config does some custom stuff on sys.path, or you have some other mechanism such that it would find your custom module. But the idea is to unify that, to have a unified way for everyone, and it takes no extra effort to think about where to put the module and how to configure it such that you can use it in the config.

Also, the other main idea is that you enforce specifying explicitly the version, such that a given recipe can never possibly break.

We should even not allow Git tags.

Because we don't rely on people not force pushing tags? We usually work with tags, so this would have been the natural way for us. But anyway, in a shared repo would anyone add the tags he needs? This might become messy.

You can encode that in the file name. E.g. if you currently have a tag like v2-custom-test13 or so, you could just have a file transformer-v2-custom-test13.py in the repo instead.

I like to have a common format for the version, like <date>-<commitrev>. I think this tells you more than a tag. If we also would allow tags there, this somewhat becomes inconsistent. Maybe it could be like <date>-<tag>. I don't know... Maybe tags are fine.

[dthulke]

Do I understand it correctly that when I import a config all parameters of the imported config are applied to my current config? If yes, an option for partial imports (like from a import b in python) would be useful if I for example only want to import the function make_trafo_block. Otherwise this may result in unexpected side effects / hard to debug issues when other default parameters are overwritten by the config.

E.g. extend the ref by a date, so you could write "2019-04-04--dc363cb` or so. Maybe we should even enforce that.

I would not enforce to extend the ref by a date. Copying the git hash is quite easy but adding the correct date imho just adds additional hassle. Also, in most cases it may be more relevant whether there are updates to the repo since this version than the date itself (which could be added as an info when running the config).

The rest sounds good to me so far.

[albertz]

Do I understand it correctly that when I import a config all parameters of the imported config are applied to my current config?

No. It is just an import, like the Python import. I.e. import x in Python is equivalent to x = import_lib("x"). And our RETURNN import_ would kind of do the same. You would also import a module, not another config. That module might provide a function make_trafo_block. Then you could use it like:

my_lib = import_(...)
...
network = {
 ...
 "trafo": my_lib.make_trafo_block(...),
 ...
}

E.g. extend the ref by a date, so you could write "2019-04-04--dc363cb` or so. Maybe we should even enforce that.

I would not enforce to extend the ref by a date. Copying the git hash is quite easy but adding the correct date imho just adds additional hassle.

This can be automated somehow. (E.g. the usual workflow would be like I outlined above, that you work in dev mode. And it automatically shows you what to put there to fix it.)

The reasoning behind this is when you read some other such config or code, you know what this commit ref means (whether this is recent or totally outdated). Otherwise it tells you nothing on its own. You always would need to check the Git repo to know when the commit is from.

This is really only minimal more effort for writing. And note that writing is maybe like 1% of the effort, while you should try to optimize the code usually more for readability and debuggability, which is the real effort.

This is standard also e.g. in Go imports. I think they had a similar reasoning. Why should we deviate from this?

Also, in most cases it may be more relevant whether there are updates to the repo since this version than the date itself (which could be added as an info when running the config).

There always will be updates. Esp when we use a common repo like returnn-experiments, which everyone uses. More interesting maybe would be if there are updates to this specific file.

When you have some config, we always ensure that the behavior you have today/now will always be the same in the future. This was always a hard requirement for RETURNN, to not break things. So why should it tell you about new versions of some imported Transformer module or so, which might have different behavior? This should be a conscious decision by the user to switch to a new version of make_trafo_block.

[albertz]

Another thought:

Such a import_ function will break any auto-completion (at least PyCharm doesn't support sth dynamic like this, see this issue, and I doubt that any other editor would be able to support this).

But I think I know a way to also support auto-completion. We can create a (virtual) package hierarchy, like returnn.extern_.github_com.rwth_i6.returnn_experiments.v2021<...>.... This can be created virtually on-the-fly, but this would again break auto-completion. But we could also automatically create symlinks. (Some details still need to be worked out about this.) (Note that we also have the related packages returnn.extern and returnn.extern_private.)

[albertz]

One problem (or maybe not a problem):

When you import multiple modules, and they maybe have other dependencies, you get a graph. A graph, because multiple modules might have the same module as a dependency. Consider this example. In this example, you might have the same module used multiple times under different versions. In Go, it would only be used once, and esp use the highest version (of all referenced versions).

In RETURNN, I think we should not do this. Every import_ should exactly import the specified version. So we might end up with some modules imported multiple times under different versions. But I think this is not a problem.

[albertz]

Ok, there is some initial implementation now, in returnn.import_.

(Meta: I wonder, there is no such thing as to "resolve" this discussion like there is for issues. But maybe this can be considered as solved for now. Unless we want to discuss further about some specific details.)

[JackTemaki]

Sorry for joining late.

One remark, I think the default folder should better be .returnn instead of returnn, otherwise people might be confused where this folder comes from when not being familiar with this function, but still using this import when running some configs they just took from somewhere. If it starts with a dot you directly now that this is some caching/config or other program specific local data.

I tried using it, but run into a crash, from which I am not sure if the path is wrong, or there is another mistake:

~/experiments/librispeech_tts/config/__init__.py in <module>
     11 from returnn.import_ import import_
     12 
---> 13 ljspeech_config = import_("github.com/rwth-i6/returnn-experiments", "2020-TTS-LJSpeech/tacotron2_ljspeech.config", "20210302-01094be")
        global ljspeech_config = undefined
        global import_ = <function import_ at 0x7f9a0a82c400>
     14 print(ljspeech_config.optimizer)
     15 

~/src/returnn/returnn/import_/import_.py in import_(repo='github.com/rwth-i6/returnn-experiments', path='2020-TTS-LJSpeech/tacotron2_ljspeech.config', version='20210302-01094be')
     20   # `module_name` has the side effect that `import_module` below will just work.
     21   mod_name = module_name(repo=repo, repo_path=repo_path, path=path, version=version)
---> 22   return importlib.import_module(mod_name)
        global importlib.import_module = <function import_module at 0x7f9a2a55c950>
        mod_name = 'returnn_import.github_com.rwth_i6.returnn_experiments.v20210302133012_01094bef2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config'

/work/tools/asr/python/3.7.1/lib/python3.7/importlib/__init__.py in import_module(name='returnn_import.github_com.rwth_i6.returnn_experi...f2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config', package=None)
    125                 break
    126             level += 1
--> 127     return _bootstrap._gcd_import(name[level:], package, level)
        global _bootstrap._gcd_import = <function _gcd_import at 0x7f9a2a69ee18>
        name = 'returnn_import.github_com.rwth_i6.returnn_experiments.v20210302133012_01094bef2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config'
        level = 0
        package = None
    128 
    129 

/work/tools/asr/python/3.7.1/lib/python3.7/importlib/_bootstrap.py in _gcd_import(name='returnn_import.github_com.rwth_i6.returnn_experi...f2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config', package=None, level=
0)

/work/tools/asr/python/3.7.1/lib/python3.7/importlib/_bootstrap.py in _find_and_load(name='returnn_import.github_com.rwth_i6.returnn_experi...f2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config', import_=<function
 _gcd_import>)

/work/tools/asr/python/3.7.1/lib/python3.7/importlib/_bootstrap.py in _find_and_load_unlocked(name='returnn_import.github_com.rwth_i6.returnn_experi...f2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config', import_=
<function _gcd_import>)

ModuleNotFoundError: No module named 'returnn_import.github_com.rwth_i6.returnn_experiments.v20210302133012_01094bef2761.2020_TTS_LJSpeech.tacotron2_ljspeech_config'

The checkout worked correctly, i confirmed that the file exist locally.

[JackTemaki]

I still don't understand the exact problem. You need to look it up anyway to know the arguments for the import_ call?

You need to look up less for the import_ call than for the import itself, but anyway this is not an important issue, just a minor thing.

Why do you need to execute Sisyphus? You just need to execute the import_ in some way. You surely don't need Sisyphus for that?

The imports should be managed by Sisyphus, so at least I need to run the full "config" part. If I do this via the Sisyphus manager or in some other way does not matter. In some cases this is possible, in some cases not directly (I have some configs that open hard coded paths outside the setup itself). I can not easily replicate this on my local machine, and I also do not want to do that.

I will try find a workaround for this issue, and just add a PR to be able to also specify the import_path via an env-var. This should be sufficient.

[albertz]

You need to look up less for the import_ call than for the import itself, ..

I don't understand. What do you mean by "look up less"? When you look up the commit, you always see the full 40-digits hash and full date. There is no "less".

The imports should be managed by Sisyphus,

I don't understand what you mean by that. What management is needed there? And why should that be needed? The imports should be independent from Sisyphus.

so at least I need to run the full "config" part. If I do this via the Sisyphus manager or in some other way does not matter. In some cases this is possible,

I don't understand. This should be always possible. You could also write a simple script which parses out all the import_ statements and runs them for you, which is totally independent from anything (except RETURNN).

in some cases not directly (I have some configs that open hard coded paths outside the setup itself). I can not easily replicate this on my local machine, and I also do not want to do that.

Why do you need to execute the config? You only need to execute the import_s, nothing else.

Also, I would argue, it's a very bad idea if the config opens other hard coded paths (while loading the config). You should always write a config in such a way that you can always load it. Sure, your dataset might be configured to some path on disk, but still loading the config should always work, only maybe running the experiment will not.

[JackTemaki]

I don't understand. This should be always possible. You could also write a simple script which parses out all the import_ statements and runs them for you, which is totally independent from anything (except RETURNN).

Yes, this would be a valid workaround

Also, I would argue, it's a very bad idea if the config opens other hard coded paths (while loading the config). You should always write a config in such a way that you can always load it. Sure, your dataset might be configured to some path on disk, but still loading the config should always work, only maybe running the experiment will not.

Why should I make my life difficult on purpose? The config will read repository paths from the settings.py of a setup (so the paths are not directly hardcoded in the config itself, but are read from the settings file). The repositories will be scanned for certain parameters, to guarantee certain checks, and also certain hashing abilities (which need to be considered before constructing the graph). I definitely want to keep those checks. Of course all this can be replicated on my local machine with a different settings file, but I really do not see the need for that.

Maybe to add: I just stated what features I would like to have, so I think it is not a good idea to conclude this discussion with: "your setup is wrong". Even if this is true, this might have historical reasons. I will not rewrite my setup over weeks, if a simple flag for relative symlinks would be sufficient to solve all of my problems at once.

[albertz]

Also, I would argue, it's a very bad idea if the config opens other hard coded paths (while loading the config). You should always write a config in such a way that you can always load it. Sure, your dataset might be configured to some path on disk, but still loading the config should always work, only maybe running the experiment will not.

Why should I make my life difficult on purpose?

It rather sounds like you are currently doing it in a very difficult and complex way, while it could be much simpler?

The config will read repository paths from the settings.py of a setup (so the paths are not directly hardcoded in the config itself, but are read from the settings file). The repositories will be scanned for certain parameters, to guarantee certain checks, and also certain hashing abilities (which need to be considered before constructing the graph). I definitely want to keep those checks. Of course all this can be replicated on my local machine with a different settings file, but I really do not see the need for that.

But this should not be done when loading the config. Loading the config should not have side effects, and should be efficient.

(There is the argument that you should not allow for any code execution in config files. That is why some people really dislike using Python for config files, and prefer Yaml, Json or whatever. Or at least a safe dialect like Starlark (used by Bazel for configs), where it is impossible to access the filesystem, network, or anything else. For simplicity and convenience reasons, we are not using those, but I would argue that we should still follow these principles.)

I also don't quite understand your argument. Surely such checks can be done in a different and better way? E.g. when specifying the dataset, you can make that easily dynamic, like:

train = get_train_dataset

Or maybe:

train = librispeech(path=lambda: ...)

Maybe to add: I just stated what features I would like to have, so I think it is not a good idea to conclude this discussion with: "your setup is wrong". Even if this is true, this might have historical reasons. I will not rewrite my setup over weeks, if a simple flag for relative symlinks would be sufficient to solve all of my problems at once.

We should definitely not make the RETURNN code unnecessarily more complicated and more prone to errors, for a new feature, just to support some historic use cases, which should be better rewritten. With such argument, you soon have a lot of unmaintainable messy code. There is already a lot of code in RETURNN which cannot easily be removed or cleaned up because it is already used now (sometimes in strange ways). Having an extra flag to make relative symlinks is also such an example. This should not be done.

But also, I still don't understand at all what problems you have. Whatever it is, it really should be extremely trivial to solve, without introducing strange hacks, workarounds, or anything like that.

We are still talking here about how to execute some import_ calls, to make auto-completion work? This is trivial.

[JackTemaki]

We are still talking here about how to execute some import_ calls, to make auto-completion work? This is trivial.

Yes sorry, that derailed a little bit. The pipeline I am talking about actually uses a json config file to define the parameters, and the pipeline design itself is part of the "Sisyphus config" . Maybe the term "config" is a little bit confusing here if you are not familiar with the structure of the Sisyphus setups (which can be designed in possibly unlimited ways). Anway this is very offtopic here.

It boils down to the fact that we currently already have modules (functions, classes etc) inside the Sisyphus setups, that we can replace by such import commands. Also, you are right that there is no problem doing that, and it is only about the autocompletion.

For most use cases the functionality is completely fine as it is (except for some minor inconveniences, but it seems I was not able to express sufficiently what I mean). For me this was only about the way the caching works, which can be made slightly more flexible from my point of view. In the case of GO, the GOPATH can be freely defined, and I think the contents will be consistent in itself, no matter where you move it and from where you access it. This is all that I wanted for the Returnn import functionality as well.