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

Use the new TranslationResult properties from ctranslate2 #404

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

pirhoo
Copy link
Contributor

@pirhoo pirhoo commented Mar 20, 2024

Hello,

I noticed a deprecation warning when running the FewShotTranslation.apply_packaged_translation method:

DeprecationWarning: Reading the TranslationResult object as a list of dictionaries is deprecated and will be removed in a futur version. Please use the object attributes as described in the documentation: https://opennmt.net/CTranslate2/python/ctranslate2.TranslationResult.html

This could lead to an exception:

> translated_tokens += translated_batch[i]["tokens"]                                                             
SystemError: <built-in method __getitem__ of PyCapsule object at 0x7d8642cd7d80> returned a result with an exception set 

So I simply used the new properties as suggested by ctranslate2.

Let me know if you want me to add a safer access to the properties for retro-compatibility.

Thanks for the great tool!

@pirhoo
Copy link
Contributor Author

pirhoo commented Apr 24, 2024

Hello 👋

Just wanted to check if you could review my PR ; Unfortunately this error is a bit blocking for me.

@PJ-Finlay PJ-Finlay merged commit b505216 into argosopentech:master Apr 30, 2024
1 check passed
@argosopentech
Copy link
Owner

Good catch thank you!

@argosopentech
Copy link
Owner

I pushed this change in v1.9.5

@pirhoo
Copy link
Contributor Author

pirhoo commented Apr 30, 2024

Wonderful, thanks!

@PJ-Finlay
Copy link
Collaborator

Why was this causing issues for you? This fix is definitely correct but I've never seen the deprecated version cause problems.

PJ-Finlay added a commit to LibreTranslate/LibreTranslate that referenced this pull request Apr 30, 2024
- Upgrade CTranslate2 to v4 for CUDA 12 support (argosopentech/argos-translate#404)
- Fix deprecation error for CTranslate2 TranslationResult (argosopentech/argos-translate#404)
@pirhoo
Copy link
Contributor Author

pirhoo commented May 1, 2024

The error started to appear for me when upgraded Argos Translate from 1.7 to 1.9: ICIJ/es-translator@a2e329b

As you can see, CTranslate2 is a transitive dependency for me. No idea why the error did not appear sooner (I saw you upgraded it prior to Argos Translate 1.8), really weird!

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.

3 participants