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

Uses python -m pip style for installation #177

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

kushaldas
Copy link
Contributor

Updates the readme to use python -m pip install style for installing python packages. This is what we suggest at upstream to reduce the confusion of which Python interpreter is being used.

See more: https://snarky.ca/why-you-should-use-python-m-pip/

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 4, 2023
@carlthome
Copy link
Contributor

I wanted to say "not needed because virtualenv" but that link was pretty convincing.

@kushaldas
Copy link
Contributor Author

I hope one of the maintainers will see and approve. Should not take that much time as it is a much simple PR without code change.

Copy link
Contributor

@JadeCopet JadeCopet left a comment

Choose a reason for hiding this comment

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

Thanks, sounds reasonable to me.

@adefossez
Copy link
Contributor

@kushaldas sorry about that, but due to a bug in HF Transformers, CI is currently failling (not your fault). However, you would have to pull and merge from main and push again before we can accept.

@JadeCopet
Copy link
Contributor

Can you ensure you're up to date with respect to main? There were some issues with Hugging Face Transformers library that required extra dependencies that is now fixed in main. Then we'll be able to merge. Thank you!

@kushaldas
Copy link
Contributor Author

I updated the PR last week. Commenting as just in case :)

@carlthome
Copy link
Contributor

Some (unrelated?) lint error on CI:

audiocraft/modules/conditioners.py:607: error: Argument 1 to "apply_model" has incompatible type "Union[Tensor, Module]"; expected "Union[BagOfModels, Union[Demucs, HDemucs, HTDemucs]]" [arg-type]

@kushaldas
Copy link
Contributor Author

Can anyone please tell me what needs to be done here?

@kushaldas
Copy link
Contributor Author

Woo hoo, can anyone please rerun the CI job and then accept the PR?

@kushaldas
Copy link
Contributor Author

Will these CI jobs ever finish?

@kushaldas
Copy link
Contributor Author

@adefossez @JadeCopet can anyone of you please run/kick the CI as required?

@carlthome
Copy link
Contributor

Since @adefossez has left Meta for a new industry lab, I imagine we won't see much reviewing here. Would be nice to know the future of AudioCraft as a package!

@kushaldas
Copy link
Contributor Author

Who can merge?!!! :)

@JadeCopet
Copy link
Contributor

Hey @kushaldas, sorry for the late response. I had to fix the CI jobs on main, you can merge our main and then we should be good to land this.

Updates the readme to use `python -m pip install` style for
installing python packages. This is what we suggest at upstream
to reduce the confusion of which Python interpreter is being used.

See more: https://snarky.ca/why-you-should-use-python-m-pip/
@JadeCopet JadeCopet merged commit 3c41377 into facebookresearch:main Nov 21, 2023
3 checks passed
@JadeCopet
Copy link
Contributor

Thank you again!

@kushaldas
Copy link
Contributor Author

Yay!!! Thank you @JadeCopet :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants