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

expose lr scheduler fields #1320

Open
yueyinqiu opened this issue Jun 1, 2024 · 4 comments
Open

expose lr scheduler fields #1320

yueyinqiu opened this issue Jun 1, 2024 · 4 comments

Comments

@yueyinqiu
Copy link
Contributor

#1314 (comment)

@yueyinqiu yueyinqiu changed the title public fields of lr scheduler expose lr scheduler fields Jun 1, 2024
@asieradzk
Copy link

Seconding this, I would like to keep scheduler and swap out optimizer without having to instantiate a new one. This sucks for model loading.

@yueyinqiu
Copy link
Contributor Author

yueyinqiu commented Jun 21, 2024

As mentioned here #1314 (comment), although the fields would be exposed, they may not be modifiable. Actually if I were the one to decide, I'd prefer to make them read only.

Hmm... so what do you mean by "sucks for model loading", or what's the exact problem have you met?

@asieradzk
Copy link

asieradzk commented Jun 21, 2024

As mentioned here #1314 (comment), although the fields would be exposed, they may not be modifiable. Actually if I were the one to decide, I'd prefer to make them read only.

Hmm... so what do you mean by "sucks for model loading", or what's the exact problem have you met?

When user of RLMatrix creates customised deep reinforcement learning agent with their own lrscheduler I have to ask them to make new one every time they load a model... since loading model involves creating new optimizer to continue training... and I can't just swap out optimizer inside previous scheduler, I need to make new one, replace cache, all that.

Additionally it sucks that lrscheduler needs to be created AFTER optimizer making it necessary for me to capture a delegate if I want to set everything up with a single method call..

@NiklasGustafsson
Copy link
Contributor

Python can't prevent you from using fields outside of the class, but .NET can. That gives us the opportunity to be more careful about what is exposed and not. If there's a scenario where keeping the scheduler and its state, but swapping out the optimizer is useful, we can support that, but it's not as simple as making the optimizer field/property public.

There is scheduler state that depends on (at least) the number of parameter groups in the optimizer, so the scheduler state has to be reset when swapping out the optimizer. This has to be designed carefully.

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