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

Refactor the Server project #78

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

Lensual
Copy link
Contributor

@Lensual Lensual commented Jul 9, 2024

  1. [refactor] Reorganize the server project structure
  2. [feat] Add graceful exit
  3. [fix] fixed return the shadowing err variable
  4. [feat] read manifest.json to memory only, no change origin file
  5. [feat] add ManifestProvider & TtsProvider, enhance for support different TTS providers based on language selected #72
  6. [fix] kill -9 is not recommend, syscall.Kill SIGTERM instead of.

@Lensual
Copy link
Contributor Author

Lensual commented Jul 9, 2024

manifest.json 这块不是很了解,我觉得 #72 根据不同的语言来使用不同的 manifest.json 可能有一些破坏性修改。

我通过Provider的形式来选择对应的 Vendor 并 Hook manifest.json的处理过程

@Lensual
Copy link
Contributor Author

Lensual commented Jul 9, 2024

看到一处代码,不确定这个是不是bug

https://github.com/Lensual/ASTRA.ai/blob/server/server/internal/service/service.go#L266

>>> //TODO check is correct? not req.AgoraAsrLanguage?
>>> language := gjson.Get(manifestJson, `predefined_graphs.0.nodes.#(name=="agora_rtc").property.agora_asr_language`).String()
	manifestJson, err = tts.ProcessManifest(manifestJson, common.Language(language), req.VoiceType)
	if err != nil {
		slog.Error("handlerStart tts ProcessManifest failed", "err", err, "requestId", req.RequestId, logTag)
		return "", "", err
	}
...

@sunshinexcode
Copy link
Collaborator

看到一处代码,不确定这个是不是bug

https://github.com/Lensual/ASTRA.ai/blob/server/server/internal/service/service.go#L266

>>> //TODO check is correct? not req.AgoraAsrLanguage?
>>> language := gjson.Get(manifestJson, `predefined_graphs.0.nodes.#(name=="agora_rtc").property.agora_asr_language`).String()
	manifestJson, err = tts.ProcessManifest(manifestJson, common.Language(language), req.VoiceType)
	if err != nil {
		slog.Error("handlerStart tts ProcessManifest failed", "err", err, "requestId", req.RequestId, logTag)
		return "", "", err
	}
...

@Lensual
在 256 行会进行设置
if req.AgoraAsrLanguage != "" {
manifestJson, _ = sjson.Set(manifestJson, predefined_graphs.0.nodes.#(name=="agora_rtc").property.agora_asr_language, req.AgoraAsrLanguage)
}

这里再进行获取
language := gjson.Get(manifestJson, predefined_graphs.0.nodes.#(name=="agora_rtc").property.agora_asr_language).String()

逻辑上没问题

@sunshinexcode
Copy link
Collaborator

manifest.json 这块不是很了解,我觉得 #72 根据不同的语言来使用不同的 manifest.json 可能有一些破坏性修改。

我通过Provider的形式来选择对应的 Vendor 并 Hook manifest.json的处理过程

@Lensual
使用 azure TTS 和 Elevenlabs TTS 采用的 manifest.json 编排不一样, 使用了 manifest.json.example 和 manifest.json.elevenlabs.example, 根据环境变量 TTS_VENDOR_CHINESE / TTS_VENDOR_ENGLISH 来决定采用哪个.

@cyfyifanchen
Copy link
Collaborator

Keep it going, I like to see this kinds of talk.

@Lensual
Copy link
Contributor Author

Lensual commented Jul 10, 2024

也许前端提供一个 manifest.json 选项会更好一些,并且提供在线编辑的接口

@sunshinexcode
Copy link
Collaborator

也许前端提供一个 manifest.json 选项会更好一些,并且提供在线编辑的接口

@Lensual
前端目前是轻逻辑, manifest.json 目前是需要预先编排好, 需要对 RTE 整体架构有一定了解, 这块目前暂时都放在后端来处理.
不久会提供manifest.json的可视化编排工具, 就会便捷很多, 到时候会再考虑优化

@cyfyifanchen cyfyifanchen added the question Further information is requested label Jul 12, 2024
@Lensual
Copy link
Contributor Author

Lensual commented Jul 13, 2024

这个提交是不是过大了,需要拆分吗

@cyfyifanchen
Copy link
Collaborator

@Lensual Right, we updated a few things. Merge the latest changes into your branch and see if you can split the code into smaller chunks for a better review.

@Lensual Lensual marked this pull request as draft July 16, 2024 16:55
@Lensual
Copy link
Contributor Author

Lensual commented Jul 16, 2024

I was wrong, 3. [fix] fixed return the shadowing err variable is not a bug.

:P

This was referenced Jul 16, 2024
@cyfyifanchen
Copy link
Collaborator

This PR needs to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants