-
Notifications
You must be signed in to change notification settings - Fork 4
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
Comments
+1 for this
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(....) |
Note that I removed |
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. |
Another +1 for this
I think we should internally differentiate between
Maybe |
When we rename |
Maybe even though that When we use also an additional abbreviation for |
I'm currently favouring this. Although it is common in general, it is not so common for our applications. In the whole RETURNN code, So, then our convention would be:
And I think we don't need special conventions for However, that implies that we import |
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 liketorch.nn.Module
,torch.nn.Linear
, ortorch.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 thenkeras.layers.Layer
, or alsofrom tensorflow.keras import layers
and thenlayers.Dense
etc.See Trax.
In Trax, it is common to use
from trax import layers as tl
, and thentl.Embedding
,tl.Dense
,tl.Serial
, etc.See Flax.
In Flax, you see
import flax.linen as nn
and thennn.Module
.(Also see doc on
flax.lines
as evolved from earlierflax.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, likereturnn_common.models.base.Module
.So, maybe the convention:
import returnn_common as rc
? But that's bad becauserc
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
inimport 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
, likefrom returnn_common import models as rcm
? Orrmm
?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 thenlayers.Linear
etc.Along the lines, we maybe should also restructure
returnn_common
, and specificallyreturnn_common.models
, orreturnn_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 thereturnn_common.models.layers.base
. In Python, usually only packages (__init__.py
) might include things from sub-packages (sotorch.nn
includes things fromtorch.nn.modules
and that includes things fromtorch.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 liketorch.nn.modules.rnn
will not dofrom .linear import *
), although single imports are fine (you often seefrom .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 frombase
andlayers
, andlayers
andbase
are just two sub modules.Maybe
returnn_common.models
is also not so optimal as it really contains also the basic structures. Maybereturnn_common.models.base
should just move toreturnn_common.base
, andbase
already be imported as part ofreturnn_common
.Maybe we could rename
returnn_common.models
toreturnn_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 (whatIEncoder
,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 intorch.nn
.The text was updated successfully, but these errors were encountered: