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

Fix base requirements #916

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Fix base requirements #916

merged 3 commits into from
Feb 5, 2024

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Feb 5, 2024

The environment doesnt have requests installed by default anymore (unclear why) which uncovered the bug that we weren't listing it

@carmocca carmocca self-assigned this Feb 5, 2024
@carmocca carmocca merged commit d80842c into main Feb 5, 2024
9 checks passed
@carmocca carmocca deleted the carmocca/requests branch February 5, 2024 19:11
@@ -1,3 +1,2 @@
torch>=2.2.0
lightning @ git+https://github.com/Lightning-AI/lightning@ed367ca675861cdf40dbad2e4d66f7eee2ec50af
jsonargparse[signatures] # CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this here means you can now no longer use any of the scripts without installing all.txt. What is the point of making this optional if using lit-gpt needs this anyway for any meaningful use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider requirements.txt as the requirements for the package and requirements-all.txt as the requirements for the scripts.

The package (model definition, tokenizer, etc) doesn't need jsonargparse because it's not used. It's only used when called as a script

Most CLI users already had to install all requirements for sentencepiece and tokenizers

rasbt pushed a commit that referenced this pull request Mar 18, 2024
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.

2 participants