Remove dynamic imports

[alan-cooney]

🚀 The feature, motivation, and pitch

Another large win for code quality would be to remove all the dynamic imports (e.g. the @register_datapipeline, @register_orchestrator... functionality). They can be replaced with standard python static imports (i.e. import x from y).

Dynamic imports have some big disadvantages:

1. They prevent most developer tooling from working (e.g. code hints, following code, type checking...), as the language server can't follow them. We'd need an extremely good reason to break dev tooling, as it makes the library much harder to use.
2. They dramatically over-complicate what is otherwise quite a small code-base.

I suspect end-users are overwhelmingly not utilising this functionality, so whilst it is technically a breaking change, I think it's worth doing now. This is particularly true as the maintainers of v. large libs almost always come to regret dynamic imports - the above problems grow exponentially with codebase size - so I think it's worth doing now whilst we can easily do this.

Alternatives

No response

Additional context

No response

[cat-state]

I started on the yaml config removal and as I did that it lead me to feel that we should reduce the need for instantiating classes by string,train.trainer/optimizer.name/schedular.name and the examples you mentioned, instead just directly instantiating them, which is similar to what this issue proposes. Ultimately this leads to a compositional "a la carte" trlx, which is great.

However, we still want to maintain the trlx.train api, so we can't require instantiating a *Trainer/the arguments needed for it for the use cases trlx.train(config=config) covers. Also, we still want to preprocess models for layer freezing/lora/ilql heads/hydra so it's probably good for trlx to be the one instantiate them.

[LouisCastricato]

Can we find a middle ground on this? I agree with both Alan's sentiment and Aman's sentiment.

[alan-cooney]

I started on the yaml config removal and as I did that it lead me to feel that we should reduce the need for instantiating classes by string,train.trainer/optimizer.name/schedular.name and the examples you mentioned, instead just directly instantiating them, which is similar to what this issue proposes. Ultimately this leads to a compositional "a la carte" trlx, which is great.

Completely agree

However, we still want to maintain the trlx.train api, so we can't require instantiating a *Trainer/the arguments needed for it for the use cases trlx.train(config=config) covers.

It's relatively common for libraries to have a 'one-line use' feature like this, where you can train a model with just some plain-text config and one line of python. However in practice it's quite error prone, as plain-text config is an anti-pattern (see #223 ) & dynamic imports make experimenting & debugging painful.

A more principled alternative would be to e.g. get the user to instantiate the separate classes, and then use a slimmed-down train method. This takes a few more lines of code but is actually far easier to use (e.g. the code hints for config settings are easier to scroll through, and all dev tooling works). I'd suggest we resolve #223 first though and then add this in.

Also, we still want to preprocess models for layer freezing/lora/ilql heads/hydra so it's probably good for trlx to be the one instantiate them.

Makes sense - this should all be possible using just standard OOP techniques, without the need for dynamic imports.

[cat-state]

In the wild use of such: https://github.com/CarperAI/OpenELM/tree/main/trlx_example https://github.com/daia99/OpenELM/blob/main/trlx_example/trainer/accelerate_ppo_softprompt_trainer.py#L140

And yeah it seems like it should be possible to support this use case through just composition