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

Sagemaker deployment #22

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Olamyy
Copy link

@Olamyy Olamyy commented May 25, 2020

I removed the ONMT folder and added it to the requirements.txt

@ruohoruotsi
Copy link
Member

ruohoruotsi commented May 25, 2020

Since there are no automated tests or CI/CD, did you verify if the training scripts still work correctly? It would be essential if they do, as this is the primary usage of the repo (i.e. model building) thus far.

@Olamyy
Copy link
Author

Olamyy commented May 25, 2020

Yeah. I haven't tested that. Will get to it.

@ruohoruotsi
Copy link
Member

okay, you'll be entering researchy code, but you should be able to pull all the data & at least see if you enter the first epoch. Ensure you have the repos checked out correctly as in https://github.com/Niger-Volta-LTI/yoruba-adr/blob/master/src/aggregate_corpora_make_parallel_text.py#L22

If it doesn't work, it may be best to separate concerns (1) ensure training isn't broken (2) deploy

@Olamyy
Copy link
Author

Olamyy commented May 25, 2020

How about the easier alternative of keeping two branches.
I can confirm this (my) branch works for sagemaker deployment.
So, you could create a new branch and I'll merge this to it.
Then, we keep master as is until you can clear the opennmt folder yourself. I feel that'll ensure I don't break anything.

@ruohoruotsi
Copy link
Member

Sounds good, I'll make a new branch as you suggest

@ruohoruotsi
Copy link
Member

new branch here: https://github.com/Niger-Volta-LTI/yoruba-adr/tree/sagemaker-deployment

mirroring yours @ Olamyy

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