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

WIP status of the returnn_common setup #93

Open
Atticus1806 opened this issue Sep 8, 2022 · 4 comments
Open

WIP status of the returnn_common setup #93

Atticus1806 opened this issue Sep 8, 2022 · 4 comments

Comments

@Atticus1806
Copy link
Contributor

Right now the serialization part of returnn_common is marked as WIP and changes are to be expected.
I would vouch for a small (or bigger) change to this, as I tested the pipeline and use it in my thesis setups right now.

The Problem: I pulled i6_experiments and stuff broke for me. Right now I think the only thing that broke was the renaming of the recursion code, but my sis has not fully loaded yet. Even though this is the risk of working with this code, I would love to use it in my thesis and refrain from making a local copy etc.

So I am opening this issue to start a small discussion on how we can handle this or find a way arround.
My first initial two ideas (just something that came to my mind, most likely not perfect):

  1. We say: Okay this code is tested thus is ready for some kind of 1.0 and after that we change it with PRs. This would be the bigger decision, but ultimately would ensure this doesnt happen anymore.
  2. Or instead we introduce a little changelog file, where one summarizes changes (that potentially affect others like a renaming or a change of logic). For example in this case this would simply be e.g. "8.9.22: Renamed PythonEnlargeStackWorkaroundCode to PythonEnlargeStackWorkaroundNonhashedCode". If we want this could then also include another line like: "8.9.22: Added support for using "import_as"", which then includes new features. So that when someone decides to update their i6_experiments version they can have a look in this file if something breaks and easily identify the problem without looking at commits/history/or having to debug.

What do you think @albertz @JackTemaki? Any other/better ideas how to handle this?

@albertz
Copy link
Member

albertz commented Sep 8, 2022

Yes, it's good to discuss this.

I would maybe vote for sth in between:

We communicate in some way (either directly via Slack, or GitHub issue -- important thing is that the response time should be fast, within an hour or so if the person would be available) that we are considering some breaking change, before we actually make it. Maybe to further discuss it, or just as an announcement.

We additionally can make an explicit changelog. It should also have a very short guide what the user needs to change, for each breaking change.

Note that you can (and should) easily see and watch the commit history, e.g.:
https://github.com/rwth-i6/returnn_common/commits/main/nn
https://github.com/rwth-i6/i6_experiments/commits/main/common/setups/returnn_common

At some point in the future (but maybe not now), we can say that we want to have further changes only via PRs. But this does not mean that we would declare it as stable 1.0 version.

Declaring it as a stable 1.0 version implies that we will not make breaking changes anymore, only via sth like behavior_version, or we introduce new versions of functions or so (e.g. nn.reduce_v2 or whatever). So this is another further step. I think we can do this when we have used it for a while, and also some other people started using it.

@albertz
Copy link
Member

albertz commented Sep 8, 2022

As a further comment, I don't expect that there will be too many (bigger) breaking changes anymore at this point anyway. But we don't really know. We should also not refrain from breaking changes at this point when we see sth we can clean up. Now is still a good time to do breaking changes. Later this is not so easy anymore.

@Atticus1806
Copy link
Contributor Author

We communicate in some way (either directly via Slack, or GitHub issue -- important thing is that the response time should be fast, within an hour or so if the person would be available) that we are considering some breaking change, before we actually make it. Maybe to further discuss it, or just as an announcement.

This is a good idea, I think we can do it like this.

Note that you can (and should) easily see and watch the commit history, e.g.:

I did that, but e.g. here I would have to look at 6-7 commits, which if they are named properly should work, but I am not conviced we can keep that up every time :D

Lets leave this open though until @JackTemaki comes back, maybe he also has an idea.

@JackTemaki
Copy link
Contributor

@curufinwe requested that we move the serialization code to i6_core after a review process. After that it will definitely be no longer WIP, but we might keep the old code around (flagged deprecated) for as long as needed if there are severe mismatches to the new version.

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

3 participants