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

Migrate away from tf1 #125

Closed
timokau opened this issue May 29, 2020 · 6 comments · Fixed by #191
Closed

Migrate away from tf1 #125

timokau opened this issue May 29, 2020 · 6 comments · Fixed by #191
Labels
enhancement New feature or request Priority: Medium

Comments

@timokau
Copy link
Collaborator

timokau commented May 29, 2020

There has been some internal discussion about this, but I think its time to also open an issue about it. We are still using tensorflow 1, which has been outdated for a while now. Switching to tensorflow 2 would be a significant effort, since the underlying model fundamentally changed (there is no explicit graph construction anymore). At that point, it may be worth evaluating switching to pytorch instead. pytorch is a newer, very popular autodiff framework.

This article comes to the conclusion that

TensorFlow is still mentioned in many more job listings that PyTorch, but the gap is closing. PyTorch has taken the lead in usage in research papers at top conferences and almost closed the gap in Google search results. TensorFlow remains three times more common in usage according to the most recent Stack Overflow Developer Survey.

Here's another relevant article. Overall it seems to me that pytorch is the more future-proof choice, and if we're going to have to rewrite a lot of the code anyway we might as well switch. I do not have any practical experience in pytorch yet though, that's just what I could determine from other's opinions and first impressions.

We should also think about how we want to do the transition. This is a major undertaking and probably will take a while. Should we support tf1 and newthing in parallel? Gradually move models to newthing (thereby having mixed support)? Fork the project? Work on one big PR/branch, effectively blocking most other work for the time due to potential conflicts?

@timokau
Copy link
Collaborator Author

timokau commented May 30, 2020

@kiudee pointed me to https://github.com/skorch-dev/skorch on #119. We could also build on top of that library, which may do some of the work for us so that we only have to specify the model structure. I'm not entirely sure how much it gives us over directly interacting with scikit-learn though.

@timokau
Copy link
Collaborator Author

timokau commented Oct 2, 2020

I feel like we can't put this off much longer. tf1 is becoming more and more outdated. It doesn't make much sense to continue with #116, since most of the changes are model-specific details now which might change anyway with a partial rewrite.

I have been looking into pytorch and tf2 a bit. I'm tending slightly towards pytorch. I could try to rewrite a part of our codebase (I'm thinking feta_linear and the derived learners) in both pytorch and tf2, then we decide on one. What do you think about that course of action @kiudee?

@kiudee
Copy link
Owner

kiudee commented Oct 13, 2020

I agree - the dependency restrictions also become cumbersome to work with (other libraries may require more recent versions).
Would you say we are ready to make a release for this version of the library?

I would also tend towards pytorch. In addition we can simplify some of the learners quite a bit using convolutions. Here is some experiment I did with FATE:

class FATE(nn.Module):
    def __init__(
        self,
        set_encoder,
        n_input_features,
        n_set_features=16,
        n_scorer_layers=1,
        n_scorer_dim=16,
        n_scorer_output=2,
        set_encoder_args=None,
        **kwargs
    ):
        super().__init__()
        if set_encoder_args is None:
            set_encoder_args = dict()
        self.set_encoder = set_encoder(
            input_channels=n_input_features,
            output_channels=n_set_features,
            **set_encoder_args
        )
        full_dim = n_input_features + n_set_features
        layers = [nn.Conv1d(full_dim, n_scorer_dim, 1), nn.ReLU(inplace=True)]
        for _ in range(n_scorer_layers - 1):
            layers.extend(
                (nn.Conv1d(n_scorer_dim, n_scorer_dim, 1), nn.ReLU(inplace=True))
            )
        self.scorer = nn.Sequential(
            *layers, nn.Conv1d(n_scorer_dim, n_scorer_output, 1)
        )

        for m in self.modules():
            if (
                isinstance(m, nn.Linear)
                or isinstance(m, nn.Conv2d)
                or isinstance(m, nn.Conv1d)
            ):
                init.xavier_uniform_(m.weight)
                if m.bias is not None:
                    m.bias.data.zero_()

    def forward(self, x, n_points=None):
        n_batch, n_obj, n_feat = x.shape
        new_x = x.transpose(1, 2).contiguous()
        embedding = self.set_encoder(new_x, n_points)
        if isinstance(embedding, tuple):
            embedding = embedding[0]
        emb_repeat = embedding.view(*embedding.shape, 1).repeat(1, 1, n_obj)
        new_x = torch.cat([new_x, emb_repeat], dim=1)
        scores = self.scorer(new_x)
        return scores.transpose(1, 2).contiguous()

@timokau
Copy link
Collaborator Author

timokau commented Oct 15, 2020

Would you say we are ready to make a release for this version of the library?

Yes, but there have been breaking changes so it would have to be a major release.

In addition we can simplify some of the learners quite a bit using convolutions.

Yeah there is a lot of potential for simplification. Your experimental code is interesting, I'll see if I can incorporate some of that into my work. In #164 I'm experimenting with a composition style, where your can specify different kinds of (rank-n) utility functions and have different classes to compose them. I think that might make things simpler and end up in a nicer structure than the current inheritance-based one.

@kiudee
Copy link
Owner

kiudee commented Oct 16, 2020

Yes, but there have been breaking changes so it would have to be a major release.

Yes, I think this is warranted. I would say we should create a release candidate before finalizing it and test it.

@kiudee kiudee removed this from the 2.0 milestone Oct 16, 2020
@timokau
Copy link
Collaborator Author

timokau commented Nov 5, 2020

Made a mistake in the annotation of #170, this is not fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority: Medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants