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

Windows compatibility #136

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HighPriest
Copy link

@HighPriest HighPriest commented Feb 16, 2024

✨ Description

As this is a Python application, there shouldn't be a reason for it to not be OS agnostic. Hence this PR.

🚧 Related Issues

  • NONE

πŸ‘¨β€πŸ’» Changes Proposed

  • Replace bash scripts with python
  • Replace delimiter/regex based path parsing, with pythons "os" module.
  • Handle python-espeak on other OSes

πŸ§‘β€πŸ€β€πŸ§‘ Who Can Review?

πŸ›  TODO

  • Replace all the bash run scripts with python versions, not only VALLE & Libritts combo
  • Make sure os agnostic path handling is done for everything else, other than VALLE & Libritts combo
  • Find out how to run python-espeak on Windows & MAC
  • Make this configuration run on Windows to completion (for now it's stuck on Extracting phoneme sequence for libritts)

βœ… Checklist

  • Code has been reviewed
  • Code complies with the project's code standards and best practices
  • Code has passed all tests
  • Code does not affect the normal use of existing features
  • Code has been commented properly
  • Documentation has been updated (if applicable)
  • Demo/checkpoint has been attached (if applicable)

So now it can be executed on windows
NOTE: This is not final! Only things relevant to running combination of VALLE & libritts!
@HarryHe11
Copy link
Collaborator

Hi @HighPriest, Thank you very much for submitting this valuable PR! Please don't hesitate to engage in further discussions with @RMSnow, @lmxue, and @VocodexElysium regarding any potential changes to this PR.

@HarryHe11 HarryHe11 requested review from RMSnow, lmxue and VocodexElysium and removed request for RMSnow February 16, 2024 10:20
@RMSnow
Copy link
Collaborator

RMSnow commented Feb 22, 2024

Hi @HighPriest, thanks for your efforts! I reviewed your code. The design including replacing .sh to .py and refine the os path operation is worth of following by Amphion.

Amphion team has been actively exploring the best ways to support our Windows users. One viable approach is certainly the one you've proposed, which allows users to utilize their local windows machines to train a model from scratch. We are also considering alternative solutions such as that we only support basic education features such as visualization tools, Jupyter Notebook, interactive demo, and pretrained models for users. For those who want to utilize their local GPU, we provide only the tutorial of Linux-based virtual machine installation, and sill recommend them to use Linux platform to train neural networks. This is a challenging issue that touches on the core philosophy of our product, and we are yet to reach a definitive decision.

We truly value your suggestions and efforts to enhance the functionality of Amphion. As an interim solution, we will recommend Windows users to leverage your branch in this PR as a patch. Thank you so much for your contribution!

@HighPriest
Copy link
Author

@RMSnow
Please do not encourage anyone to use this PR as a functional solution for windows.

This PR has been marked as a draft! It doesn't complete the first step (features extraction), because of missing dependency on Windows.

@RMSnow
Copy link
Collaborator

RMSnow commented Feb 22, 2024

@HighPriest Oh, Sure! Thank you all the same :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants