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

Add translation pipeline parameter to return selected models and detected language #424

Merged
merged 4 commits into from
Feb 11, 2023

Conversation

saucam
Copy link
Contributor

@saucam saucam commented Feb 9, 2023

Added a debug flag to translation pipeline, which when passed, will return detected language and the model used for translation.

Resolves #383

@saucam
Copy link
Contributor Author

saucam commented Feb 9, 2023

@davidmezzetti, request you to please review and pardon my naive mistakes for the first PR !

Copy link
Member

@davidmezzetti davidmezzetti left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. It looks great!

Just a couple comments/clarifications/suggestions above. I also just kicked off the build to check on test coverage.

@saucam
Copy link
Contributor Author

saucam commented Feb 9, 2023

Thanks for the review and detailed feedback. I have made the changes and replied to the comments. Do take a look.

@davidmezzetti davidmezzetti added this to the v5.4.0 milestone Feb 10, 2023
@davidmezzetti davidmezzetti merged commit 0ce31ee into neuml:master Feb 11, 2023
@davidmezzetti
Copy link
Member

Thanks again, this was merged in. The build error was unrelated to your PR.

@davidmezzetti
Copy link
Member

davidmezzetti commented Feb 11, 2023

One additional thing, if you want to show up as a contributor on the project (this page https://github.com/neuml/txtai/graphs/contributors), you'll need to add your school email as a backup email on your GitHub account as that was the email associated with the commits.

You can read more here: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

@saucam
Copy link
Contributor Author

saucam commented Feb 11, 2023

@davidmezzetti thank you soo much for all the help and the merge.

Actually I already have my school email set as backup email in my account, can see the screenshot below:

Screenshot 2023-02-11 at 11 43 24 PM

Is it because the contributors page does not include merge commits?

"Contributions to master, excluding merge commits and bot accounts"

@davidmezzetti
Copy link
Member

No problem. I see the issue, the email associated with the commits is different that than. It has your full name instead of last name initial.

If you run git log locally on the repo, you'll see the email that was used.

@saucam
Copy link
Contributor Author

saucam commented Feb 11, 2023

oops sorry for this, any way to fix that after the merge? I fixed it in my branch though.

@saucam
Copy link
Contributor Author

saucam commented Feb 11, 2023

@davidmezzetti could you also point me to maybe the next change that I could help with?
Also, does it make sense to use squashing while merging the PRs? That way the history is much cleaner.

@davidmezzetti
Copy link
Member

The easiest thing to do would be adding that email to your profile. Rewriting the commit history would likely cause more problems than it's worth as anyone who's pulled since would have conflicts. Certainly change your email moving forward though in your .git/config file. Setting your email visibility to private and using a noreply.github.com address is another option to consider for future commits.

In terms of squashing commits, sometimes I do that, especially when someone submits a PR with a lot of irrelevant changes. But in your case, since you worked with me back and forth, I thought it made sense to keep that history (and credit more commits to you).

Regarding additional PRs, I marked a few as "Good First Issues". #423 would naturally build off what you just did with the translation pipeline. #406 is another good one to try.

We have a Slack channel that would be good to join if you plan to continue contributing: https://join.slack.com/t/txtai/shared_invite/zt-1cagya4yf-DQeuZbd~aMwH5pckBU4vPg

@saucam
Copy link
Contributor Author

saucam commented Feb 12, 2023

ok, yes I have changed it already now. Thanks for the slack link, let me join. Thank you for the pointers on the new issues. Let me try 423 first

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.

Add translation pipeline parameter to return selected models
2 participants