CarperAI / CarperAI/trlx

Remove dynamic imports

Open
#277 4 comments 2 reactions 1 assignee Claimed by @jon-tow View on GitHub
feature request
Dominant language
Python
Stars
4.8k
Forks
487
PR merge metrics
No merged PRs in 30d

Description

### 🚀 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_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.