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: warn user to install mamba_ssm package #1019

Merged

Conversation

NanoCode012
Copy link
Collaborator

Fixes #975 . Warns when user does not have package installed.

@hamelsmu
Copy link
Collaborator

hamelsmu commented Dec 29, 2023

Do you think this needs to be added to requirements.txt and also on the README in the developer section?

git clone https://github.com/OpenAccess-AI-Collective/axolotl
cd axolotl

pip3 install packaging
pip3 install -e '.[flash-attn,deepspeed,mamba-ssm]'

@hamelsmu
Copy link
Collaborator

I also wonder if a mamba smoke test of some kind is a good idea?

@NanoCode012
Copy link
Collaborator Author

Do you think this needs to be added to requirements.txt

Yes, that's what I thought about as well. To what extent should we add these in? This package is optional. Should we add a table like in https://github.com/EleutherAI/lm-evaluation-harness#install to show dependency?

I also wonder if a mamba smoke test of some kind is a good idea?

What kind of test do you have in mind?

@hamelsmu
Copy link
Collaborator

For the smoke test, maybe something like we have with the tiny mistral here? I really like that test because its super fast but still catches errors. The 130M param one maybe? what do you think?

@hamelsmu
Copy link
Collaborator

Is this our first optional dependency? For example is flash attention an optional dependency too? Maybe keep it in requirements.txt for consistency right now, until we organize all the optional dependencies together somehow?

By the way, now that I think of it, I think the README is confusing because

pip3 install -e '.[flash-attn,deepspeed]'

Should really just be

pip3 install -e .

because flash-attn,deepspeed are already in the requirements.txt?

@NanoCode012
Copy link
Collaborator Author

By the way, now that I think of it, I think the README is confusing because

Ah, I asked caseus about this as well. This was due to some CI issue with installing FA and deepspeed at same time as other packages. Hence the setup.py skips these and install after the base packages. Perhaps, they should be removed from requirements.txt? What do you think is best?

@hamelsmu
Copy link
Collaborator

Hmm maybe it's best to wait for @winglian to comment then - CI issues can be nasty 😃

@winglian
Copy link
Collaborator

Oh gosh. Yeah. I struggled with the flash attn w CI for a solid day. I think we should at some point try to clean up all the dependencies, but maybe not for this particular issue. I thi knits probably fine to just make mamba a required dependency as long as it doesn't have too many version specific pins that might conflict with upgrades we'll make down the line.

@hamelsmu
Copy link
Collaborator

@NanoCode012 i approved PR so you can merge after adding the dependency to requirements.txt

@NanoCode012
Copy link
Collaborator Author

Hm, I wonder if we need to move mamba installation out, just like flash now.. It's asking for packaging, which is being installed at same time.

@winglian winglian merged commit d69ba2b into OpenAccess-AI-Collective:main Jan 10, 2024
6 checks passed
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.

Config for training Mamba breaks
3 participants