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

Configs like train_config should be named using PascalCase #722

Open
vvolhejn opened this issue Oct 10, 2024 · 2 comments
Open

Configs like train_config should be named using PascalCase #722

vvolhejn opened this issue Oct 10, 2024 · 2 comments

Comments

@vvolhejn
Copy link
Contributor

🚀 The feature, motivation and pitch

Config dataclasses like llama_recipes.configs.training.train_config should be named using PascalCase, in this case TrainConfig. The current naming violates a widely accepted Python convention.

It also leads to patterns like from llama_recipes.configs import train_config as TRAIN_CONFIG here, where the symbol needs to be renamed upon import to avoid conflicts with a train_config variable, which is an instance of the train_config (TRAIN_CONFIG) class.

I'm happy to do the renames but I wanted to hear from the devs first. This change does break backwards compatibility but since llama-recipes is currently on version 0.0.4 I assume the whole API is still very experimental?

Alternatives

No response

Additional context

No response

@mreso
Copy link
Contributor

mreso commented Oct 10, 2024

Hi @vvolhejn thanks for bringing this up. Avoiding to break bc was what lead to this outcome. This affects all config classes and we should make these changes to all configs simultaneously. Would be happy to review a PR. I am currently working on fixing the unit tests once again so we can test the proposed changes more reliably.

@vvolhejn
Copy link
Contributor Author

Would you be open to renaming if we rename all configs at once?

Regarding testing the changes, a typechecker like Pyright would catch this – it would detect when you're trying to import something that's not defined. I'm a fan of Pyright but it does come with some overhead, like needing to keep type stubs for packages that don't define them, like transformers, or sometimes needing to typing.cast values to convince the typechecker of something.

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

2 participants