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

Fixed missing tf_predictions and save_directory variables. Split tf_ and pt_ a bit better. Added .gitignore to ignore the saved model paths. #117

Closed
wants to merge 7 commits into from

Conversation

cfregly
Copy link

@cfregly cfregly commented Nov 24, 2021

@LysandreJik
Copy link
Member

Hi @cfregly, thanks for opening a PR!

The notebooks were not synchronized to the latest master updates of transformers, which led to these errors. I'm bringing all the notebooks up to speed with master in the following PR: #118

Could you let me know if that works for you? There won't be the changes to the save_directory and the split between tf_ and pt_. If you're feeling strongly about these changes, could you apply them to this file? https://github.com/huggingface/transformers/blob/master/docs/source/quicktour.rst

I'll then re-generate the notebooks so that they contain the changes. Let me know!

@cfregly
Copy link
Author

cfregly commented Nov 25, 2021

@LysandreJik Thanks for the quick response.

the code in the .rst looks good, although I don’t see the save_directory variable being defined, but maybe I’m missing something.

Summary: As long as the generated code runs without error, I’m ok.

Actually, can you add the commented-out pip install dependencies from the top of my PR? Makes things a bit more clear.

thanks!!

@LysandreJik
Copy link
Member

Yes, the save_directory variable is not defined as mentioned in my message above; I personally think it's not an issue as it's pretty explicit, but if you feel strongly about it please do apply your changes to https://github.com/huggingface/transformers/blob/master/docs/source/quicktour.rst and I'll be happy to merge it and update the notebooks.

Please do note that the cells right after the model saving won't necessarily work out of the box as it is indicated in the notebook that some cells are only shown if the user would like to load a TF model in a PT model, or vice-versa, which is not the case if you're following the notebook from start to finish.

I confirm all other cells run to completion.

@cfregly
Copy link
Author

cfregly commented Nov 25, 2021

Gotcha.

I think it’s important for this notebook to run to completion without error. Seems like a better experience for the end user.

I’ll apply all of my changes to the .rst and we can go from there.

Thanks so much!

@LysandreJik
Copy link
Member

That sounds good, will keep an eye out for the PR. Thanks, @cfregly!

@cfregly
Copy link
Author

cfregly commented Nov 25, 2021

@LysandreJik

Ok, i've created a PR to the transformers/ repo: huggingface/transformers#14529

I'll keep an eye on that PR and delete this PR in the next couple days.

@LysandreJik
Copy link
Member

Merged your PR, updated it in huggingface/transformers#14533 and applied the changes in #118.

I had to do another follow up PR here huggingface/transformers#14534 and applied the changes in #119

@cfregly cfregly closed this Nov 26, 2021
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