Skip to content
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

Reorganization of code and module names, and user network code conventions #39

Closed
albertz opened this issue Oct 26, 2021 · 7 comments
Closed
Assignees
Milestone

Comments

@albertz
Copy link
Member

albertz commented Oct 26, 2021

We should define (or sketch) common conventions when someone uses the returnn-common network definition code, e.g. Module and other layer wrappers.


In TensorFlow, import tensorflow as tf is really standard.

In PyTorch, see examples.
PyTorch code usually always does import torch and then all user code would write out the names like torch.nn.Module, torch.nn.Linear, or torch.nn.MSELoss etc.
Sometimes you also see from torch import nn.
But I also have seen from torch.nn import Sequential, Conv2d, MaxPool2d, Linear, ReLU.
For the functional API, the convention is import torch.nn.functional as F.

In Keras, see examples.
Keras code usually does from tensorflow import keras and then keras.layers.Layer, or also from tensorflow.keras import layers and then layers.Dense etc.

See Trax.
In Trax, it is common to use from trax import layers as tl, and then tl.Embedding, tl.Dense, tl.Serial, etc.

See Flax.
In Flax, you see import flax.linen as nn and then nn.Module.
(Also see doc on flax.lines as evolved from earlier flax.nn, and this.)


So, for us:

One problem we have is that returnn_common is already long, too long that a user would always want to have that everywhere. So we will not see names fully written out, like returnn_common.models.base.Module.

So, maybe the convention: import returnn_common as rc? But that's bad because rc is too commonly used in other context.
The name should be max 3 chars (for 4 chars, e.g. PyCharm will perform spell check).
The name should not be commonly used in other context.
The name should somehow reflect "RETURNN common".
Suggestions (for X in import returnn_common as X) welcome.
As a good proxy, use GitHub code search, e.g. this. The number should not be too high. rc yields 174,541,911 code results.

Or maybe not so important, but instead some convention for returnn_common.models, like from returnn_common import models as rcm? Or rmm?

Wildcard imports (from returnn_common.models.layers import *) are bad in general for Python, not just specific here.

I guess explicitly importing things is probably fine, so when the user does from returnn_common.models.layers import Module, Linear or so.

Maybe from returnn_common.models import layers is also fine, and then layers.Linear etc.


Along the lines, we maybe should also restructure returnn_common, and specifically returnn_common.models, or returnn_common.models.layers etc. Now we still can do that.

I think it's bad that currently returnn_common.models.layers is the main import for the user, which also includes the returnn_common.models.layers.base. In Python, usually only packages (__init__.py) might include things from sub-packages (so torch.nn includes things from torch.nn.modules and that includes things from torch.nn.modules.module, torch.nn.modules.linear, etc), but you usually never see that one module includes everything from another module on the same level (eg sth like torch.nn.modules.rnn will not do from .linear import *), although single imports are fine (you often see from .module import Module).
So this definitely has to change. E.g. we could make returnn_common.models the base import for the user, which includes everything from base and layers, and layers and base are just two sub modules.

Maybe returnn_common.models is also not so optimal as it really contains also the basic structures. Maybe returnn_common.models.base should just move to returnn_common.base, and base already be imported as part of returnn_common.

Maybe we could rename returnn_common.models to returnn_common.nn. Then it becomes very similar to PyTorch or Flax.

We might also split the base modules (layer wrappers) in returnn_common.models from higher-level interfaces (what IEncoder, IDecoderLabelSyncRnn etc is supposed to become) and higher-level models (Conformer etc)? Although the separation is not always clear. E.g. LSTM, is it part of the base or higher-level? Transformer? Conformer? In PyTorch, it's all in torch.nn.

@albertz albertz changed the title User network code conventions Reorganization of code and module names, and user network code conventions Oct 26, 2021
@albertz albertz added this to the first-release milestone Oct 26, 2021
albertz added a commit that referenced this issue Oct 26, 2021
albertz added a commit that referenced this issue Oct 26, 2021
@JackTemaki
Copy link
Contributor

Maybe we could rename returnn_common.models to returnn_common.nn

+1 for this

The name should be max 3 chars

Then maybe:

from returnn_common.nn import layers as rcl
from returnn_common.nn import modules as rcm

encoder = rcm.asr.encoder.ConformerEncoder(.....)
linear = rcl.Linear(....)

@albertz
Copy link
Member Author

albertz commented Oct 26, 2021

Note that I removed returnn_common.models.layers now. I think returnn_common.models (or returnn_common.nn or returnn_common.layers) itself should cover this. In your user code, you would only use from returnn_common.modules import Linear or so.

@albertz
Copy link
Member Author

albertz commented Oct 26, 2021

from returnn_common.nn import layers as rcl
from returnn_common.nn import modules as rcm

Now you differentiate between modules (models?) and layers. As I mentioned, I'm not sure if this distinction is maybe too ambiguous in certain cases. You can also see Transformer (or a Transformer block) as a single layer, just like LSTM.

@Atticus1806
Copy link
Contributor

Atticus1806 commented Oct 26, 2021

Maybe we could rename returnn_common.models to returnn_common.nn

Another +1 for this

Now you differentiate between modules (models?) and layers.

I think we should internally differentiate between RETURNN layers and Modules which build up on them. The reason I see is that the user, if there are issues, can see wether this might be RETURNN related or something in the Modules. Splitting them by file or internal comments should be enough for this. In general I think this is not something that the users should worry about if they just want to use returnn_common.
Regarding the naming Module or Layer you can argue for both. You can see a Transformer as one layer but also one layer as a (one-layer) Module. This is just personal preference I guess.

Suggestions [for import returnn_common as X] welcome.

Maybe com, nnc or rco? those additionally to your suggestions come to mind from my head.

@albertz
Copy link
Member Author

albertz commented Oct 26, 2021

When we rename returnn_common.models to returnn_common.nn, the natural abbreviation would be rcn. Which doesn't seem common in other context, so maybe that's already fine.

@albertz
Copy link
Member Author

albertz commented Oct 26, 2021

Suggestions [for import returnn_common as X] welcome.

Maybe com, nnc or rco? those additionally to your suggestions come to mind from my head.

com is way too common. Also too less specific to RETURNN or neural nets.

nnc is not common. But I think it should start with an r to make it at least somewhat RETURNN specific.

rco is maybe ok.

Maybe even though that rc is very common, it is still fine?

When we use also an additional abbreviation for returnn_common.nn, it also should be consistent. E.g. when we use rc here, then rcn is consistent. Or when we use rco, then it would be rcon, which is maybe not nice, and also 4 chars, so PyCharm will complain due to spellcheck. Although rc.nn or rco.nn is not really much longer than rcn or rcon anyway, so we maybe don't need the abbreviation for returnn_common.nn anymore.

albertz added a commit that referenced this issue Oct 27, 2021
@albertz
Copy link
Member Author

albertz commented Oct 27, 2021

Maybe even though that rc is very common, it is still fine?

I'm currently favouring this. Although it is common in general, it is not so common for our applications. In the whole RETURNN code, rc was never used for anything.

So, then our convention would be:

import returnn_common as rc

And I think we don't need special conventions for returnn_common.nn etc. People can just use rc.nn.

However, that implies that we import nn in the root package as well. This is fine? Or should we do lazy imports, as it is common e.g. in TensorFlow or PyTorch?

albertz added a commit that referenced this issue Oct 27, 2021
albertz added a commit that referenced this issue Oct 27, 2021
albertz added a commit that referenced this issue Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants