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

Moving more modules to C#. #1259

Closed
wants to merge 217 commits into from
Closed

Conversation

NiklasGustafsson
Copy link
Contributor

Instead of creating a native-code module object, this PR is starting to move the module logic to C#, when reasonable.

Also, add missing properties for various modules -- the two go hand in hand.

NiklasGustafsson and others added 30 commits February 27, 2023 11:22
@NiklasGustafsson
Copy link
Contributor Author

Re-opening to get some builds running. Still Work in Progress.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Mar 22, 2024

I'm trying to deal with PairwiseDistance. I thought that the class should extends ParamLessModule, as AvgPool1D and some other classes do. However, I realized that there might be some problems here.

In PyTorch it is allowed to do like:

import torch.nn.utils

x = torch.nn.AvgPool1d(1)
x.register_module("???", torch.nn.Linear(1, 1))
x.to(dtype=torch.complex128)

print(x.get_submodule("???").weight.dtype)  # torch.complex128

Although I think nobody will do this... but if we want it consistent to PyTorch, then ParamLessModule._to cannot just return this.

Perhaps at least we should disable register_modules, register_parameters and register_buffer on ParamLessModule. (throwing an exception instead of working incorrectly)

@NiklasGustafsson
Copy link
Contributor Author

I think that's a great idea, but not sure about register_module() -- you could conceivably have a module that has no parameters or buffers but does have parameter-less sub-modules.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Mar 22, 2024

To be radical, we could make all the register_xxx to be internal. (Registering something extra on a built-in module? At leaset I won't do that.)

I think the only problem is that it will cause a great inconsistence with PyTorch. (However throwing an exception is also somewhat inconsistent. I'm not really sure where the boundary is.)

@NiklasGustafsson
Copy link
Contributor Author

You can always throw an exception and then relax the constraint later. I'm adding logic to enforce that submodules must also be parameter-less.

@NiklasGustafsson
Copy link
Contributor Author

You can always throw an exception and then relax the constraint later. I'm adding logic to enforce that submodules must also be parameter-less.

Pushed these changes to GH.

@@ -45,13 +65,30 @@ public abstract class ParamLessModule<T1, T2, T3> : nn.Module<T1, T2, T3>
protected internal override nn.Module _to(DeviceType deviceType, int deviceIndex = -1) => this;

protected internal override nn.Module _to(ScalarType dtype) => this;

public override void register_buffer(string name, Tensor tensor, bool persistent = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is worth adding sealed to the override?

@NiklasGustafsson
Copy link
Contributor Author

Closing it again, just to avoid a lot of CI builds. Will reopen again later.

@GeorgeS2019
Copy link

@ shaltielshmid

Do you have the link to the todoList : what yet to be done?

I hope this PR continue

@yueyinqiu
Copy link
Contributor

@GeorgeS2019
Copy link

@shaltielshmid
@NiklasGustafsson
@yueyinqiu

It is not normal for @NiklasGustafsson to be silent for so long without letting us know that he is on a long vacation.

@shaltielshmid => I hope you are in communication with him on how best to proceed with this PR.

@NiklasGustafsson
Copy link
Contributor Author

This PR is opened and closed while the ongoing work is tracked in my personal repo. Once ready for release, this PR will be reopened again.

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Apr 23, 2024

In PyTorch, buffers are not automatically registered:

import torch.nn


class MyModel(torch.nn.Module):
    def __init__(self):
        super().__init__()
        self.a_tensor = torch.zeros([])


print(len(MyModel().state_dict()))
# 0

However in current TorchSharp, RegisterComponents does that:

using TorchSharp;
using static TorchSharp.torch;

Console.WriteLine(new MyModel().state_dict().Count);
// 1

class MyModel : torch.nn.Module
{
    private Tensor aTensor;
    public MyModel() : base(nameof(MyModel))
    {
        aTensor = torch.zeros([]);
        this.RegisterComponents();
    }
}

So should we also modify that?

@yueyinqiu
Copy link
Contributor

yueyinqiu commented Apr 24, 2024

Meanwhile I suppose the call of submodule's RegisterComponents in register_module is strange. But some are true depending on that. #1272 (comment) Should we also change this?

P.S. Current ModuleList allows new items to be added after registering, but they won't appear in parameters/state_dict/... That's also somewhat unexpected and misleading.

@lintao185
Copy link

How is this task going?😊😊😊(☆▽☆)

@NiklasGustafsson
Copy link
Contributor Author

How is this task going?😊😊😊(☆▽☆)

Ah, yes... I was pulled into other tasks (TorchSharp is a background activity for me), and I only found time for smallish things, while this is a big task. One of the smaller tasks was adding the non_blocking argument to a variety of to() APIs, which meant that I now have > 80 files with merge conflicts to resolve in the branch where this work is maintained. I'll get to it sometime soon, but I think it requires a day without meetings, to just focus on the monkey-editing task of resolving merge conflicts.

@shaltielshmid
Copy link
Contributor

@NiklasGustafsson I was also pulled into quite a few other tasks these past few weeks - let me know when you've resolved the merge conflicts, and I'll finish up the last few items on our list?

@NiklasGustafsson
Copy link
Contributor Author

I can probably set aside some time tomorrow to work in the merge conflicts. With a little bit of luck, it'll be pretty mechanical.

@NiklasGustafsson
Copy link
Contributor Author

NiklasGustafsson commented Jun 7, 2024

I've taken care of the merge conflicts. I'm going to spend a bit of time on the default device issue that isn't completely working, it seems, and then I'll do a release.

@yueyinqiu yueyinqiu mentioned this pull request Jun 20, 2024
16 tasks
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

Successfully merging this pull request may close these issues.

6 participants