Skip to content

fix: replace deprecated translation methods - #6567

Merged
juliusknorr merged 1 commit into
mainfrom
replace-deprecated-translation-methods
Mar 11, 2025
Merged

fix: replace deprecated translation methods#6567
juliusknorr merged 1 commit into
mainfrom
replace-deprecated-translation-methods

Conversation

@luka-nextcloud

@luka-nextcloudluka-nextcloud commented Oct 28, 2024

Copy link
Copy Markdown
Contributor

📝 Summary

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

✔️ TODO

  • Test this with an actual translation ai setup

mejo-
mejo- previously requested changes Nov 4, 2024

@mejo-mejo- left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't dive too deep into the changes, but a quick testing didn't work as expected. While the fake providers provided by the testing app worked, the providers provided by the translation app were not listed.

What I did to reproduce:

  1. Disable app testing
  2. Enable apps assistant and translate
  3. Run occ translate:download-models

When I load the translation modal from the assistant menu with this PR applied, the dropdowns "translate from" and "translate to" are empty and don't provide any language. Maybe I missed something?

@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

@mejo- As I understand, the assistant app cannot work without testing app (or AI provider apps). So, disabling testing app means disabling assistant and translation. So, I think the test case doesn't make sense to me.

@juliusknorr

Copy link
Copy Markdown
Member

There is also translate2, not sure if just an addition or successor. translate doesn't implement the new API.

@marcelklehr From an integration team perspective, is it work to keep both APIs available or is just going with the task processing API and ignoring the old translation API fine.

@juliusknorrjuliusknorr added the bug Something isn't working label Nov 21, 2024
@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

In my opinion, we should go with the task processing API and ignore the old translation API since it might be complicated to maintain when keeping both APIs.

@marcelklehr

marcelklehr commented Nov 21, 2024

Copy link
Copy Markdown
Member

We opted to implement neither forward nor backward compatibility for the old Translation API in TaskProcessing. The translate2 app only uses the TaskProcessing API, while the translate app uses the old Translation API. I'd recommend to transition to TaskProcessing directly if you don't need to support nc < 30, as the old Translation API is already deprecated and you will need to transition at some point. If it's too much work for you to add support for task processing, we can talk about adding forward and backward compatibility in server, but I'd rather avoid that.

@luka-nextcloudluka-nextcloud self-assigned this Nov 25, 2024
@mejo-

Copy link
Copy Markdown
Member

@luka-nextcloud I'm still unable to successfully test this using real translation providers. Did you test this with real providers, or just with the testing app?

What I did:

  1. Disable app testing
    1. Setup app_api environment as documented in https://docs.nextcloud.com/server/latest/admin_manual/ai/app_api_and_external_apps.html
  2. Setup translate2 app as described in https://docs.nextcloud.com/server/latest/admin_manual/ai/app_translate2.html
  3. Build this branch, select something in text and open the translate assistant modal.

Problems:

  • I cannot select a source language except "detect automatically"
  • Selecting "translate" does nothing.

When opening the assistant modal directly in the header bar (not via Text), translation works as expected, so I guess there's something wrong with the implementation in Text.

This is how the dropdowns for selecting languages in the translation modal via Text look in my tests:

grafik
grafik

@luka-nextcloud
luka-nextcloudforce-pushed the replace-deprecated-translation-methods branch from e879703 to 08e25e1CompareDecember 4, 2024 11:46
@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

@mejo- I have tested with both apps testing and translate2. BTW, if we use translate2, the from-language selection will only have 1 option "Detect Language"

@marcelklehr

Copy link
Copy Markdown
Member

Note: integration_openai also implements a translation provider

@max-nextcloud

Copy link
Copy Markdown
Collaborator

/backport! to stable31

@marcelklehr

Copy link
Copy Markdown
Member

Installed successulfy now. Translation menu entry in the AI menu doesn't do anything on click:

Error: Could not find initial state translation_can_detect of text
e http://localhost:8080/apps/text/js/index-CJ7wDv5l.chunk.mjs:1
data http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
vs http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
ds http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
ls http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
_init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
s http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
Za http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
d http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
E http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
fc http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
_update http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
$mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
init http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
c http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
i http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Jg http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
_update http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
u http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
get http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
run http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Th http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Zn http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
_s http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
[vue.runtime.esm-DuKaSI2H.chunk.mjs:1:25024](http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs)
TypeError: this.languages is undefined
fromLanguages http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
evaluate http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
lr http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
kA http://localhost:8080/apps/text/js/Editor-DCXGGTKZ.chunk.mjs:2
_render http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
$mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
init http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
d http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
E http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
p http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
fc http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
_update http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
r http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
get http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
t http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
Va http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
$mount http://localhost:8080/apps/text/js/vue.runtime.esm-DuKaSI2H.chunk.mjs:1
init http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
c http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
i http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Jg http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
_update http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
u http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
get http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
run http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Th http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
Zn http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2
_s http://localhost:8080/apps/viewer/js/logger-D5E9ePnd.chunk.mjs:2

@juliusknorr

Copy link
Copy Markdown
Member

I can share credentials for easier testing with the integration_openai provider, just ping me if needed

@max-nextcloud

Copy link
Copy Markdown
Collaborator

@marcelklehr

Installed successulfy now. Translation menu entry in the AI menu doesn't do anything on click:

That looks like you did not compile it yet.
I told you you would not need to in chat - but I did not understand you were working on a feature branch / pull request. In text we have a compilation task that runs on the main and stable?? branches and provides the compiled js.

Could you try again? We can also jump on a call next week to try it together.

@marcelklehr

Copy link
Copy Markdown
Member

Yeah, let's do that :)

@marcelklehr

Copy link
Copy Markdown
Member

notes from test with new build:

On opening:

TypeError: this.toLanguage is null
fromLanguage Translate.vue:157

when selecting a new "from language":

TypeError: newVal is null
fromLanguage Translate.vue:157
  • Cannot retranslate after selecting a new "from language"

@max-nextcloudmax-nextcloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tested this with @marcelklehr and the integration worked fine. There were two errors in the logs:

  • When changing the fromLanguage while toLanguage was not set yet
  • When clearing the toLanguage'.

Both pointed to line 157 of Tranlate.vue. I'll propose a change that will fix them.

In addition it would be good to also clear the result when the fromLanguage is changed - to allow for a recomputation.

Other than that this looks fine. I'll approve so you can fix the remaining things and then merge @luka-nextcloud

Comment threadsrc/components/Modal/Translate.vue
@max-nextcloudmax-nextcloud removed their assignment Feb 27, 2025
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@luka-nextcloud
luka-nextcloudforce-pushed the replace-deprecated-translation-methods branch from 08e25e1 to c87a4b8CompareMarch 10, 2025 10:33
@codecov

codecovBot commented Mar 10, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.

Project coverage is 37.51%. Comparing base (7d9b968) to head (c87a4b8).
Report is 14 commits behind head on main.

Files with missing linesPatch %Lines
src/components/Modal/Translate.vue0.00%38 Missing ⚠️
src/components/Assistant.vue0.00%1 Missing ⚠️
src/components/Menu/MenuBar.vue0.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #6567 +/- ##
==========================================
+ Coverage 37.49% 37.51% +0.02% 
==========================================
Files 943 933 -10 Lines 42501 42476 -25 Branches 1466 1456 -10 ==========================================
Hits 15935 15935 + Misses 25788 25773 -15 + Partials 778 768 -10 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luka-nextcloud

Copy link
Copy Markdown
ContributorAuthor

@mejo- Could you please give an approval if everything is okay?

@juliusknorr

Copy link
Copy Markdown
Member

Lets get this in as @mejo- is unavailable

@juliusknorr

juliusknorr commented Mar 10, 2025

Copy link
Copy Markdown
Member

@luka-nextcloud Can you double check that this is somehow covered with tests? We should add some if we don't have yet

@luka-nextcloud
luka-nextcloud dismissed mejo-’s stale reviewMarch 10, 2025 13:34

Max tested it and it worked for Marcel as well.

@juliusknorr
juliusknorr merged commit bb4ce6c into mainMar 11, 2025
@juliusknorr
juliusknorr deleted the replace-deprecated-translation-methods branch March 11, 2025 19:07
@github-project-automationgithub-project-automationBot moved this from 👀 In review to ☑️ Done in 📝 Productivity teamMar 11, 2025
@juliusknorr

Copy link
Copy Markdown
Member

/backport to stable31

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewbugSomething isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Replace deprecated translation methods

5 participants

@luka-nextcloud@juliusknorr@marcelklehr@mejo-@max-nextcloud