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

cmakeとsetup.pyを切り離す、テストの追加 #51

Merged
merged 14 commits into from
Dec 17, 2021

Conversation

Yosshi999
Copy link
Contributor

内容

  • これまでpip install時に自動でcmakeが走っていたが、これを取りやめてcmake buildを手動でやるようにする
  • cpu版/cuda版を統一し、ダウンロードしてきたonnx runtimeがcudaに対応しているかどうかを実行時に確認するように変更
  • 単体テストをgithub actionに追加

関連 Issue

close #50
ref #16 : cudaをサポートしているかどうかを確認する supports_cuda という関数を実装したので、良さそうならこれをAPIに追加しようと思います

@Yosshi999
Copy link
Contributor Author

ref #15

@Hiroshiba Hiroshiba requested review from Oyaki122 and Hiroshiba and removed request for Oyaki122 December 15, 2021 05:20
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!
良さそうに感じました!

core/src/core.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

あ、一箇所コメントしたからまだLGTMじゃなかった、すみません。

Copy link
Member

@Oyaki122 Oyaki122 left a comment

Choose a reason for hiding this comment

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

すでに挙げられている点以外には特に問題ないと思います

Copy link
Member

@Oyaki122 Oyaki122 left a comment

Choose a reason for hiding this comment

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

API部と取得部が分割されていてとてもいいと思います
一点だけ確認をお願いします

core/src/core.cpp Show resolved Hide resolved
core/src/core.cpp Outdated Show resolved Hide resolved
@Oyaki122 Oyaki122 self-requested a review December 16, 2021 05:23
@Yosshi999 Yosshi999 requested a review from Hiroshiba December 16, 2021 07:43
@Yosshi999
Copy link
Contributor Author

testが落ちてますね...
pip install前にnumpyとcythonを自動で入れる方法は何かありますか?

@Hiroshiba
Copy link
Member

pip install前にnumpyとcythonを自動で入れる方法は何かありますか?

おそらく無いです・・・。
インストールに必要なライブラリをまとめたrequirements.txtを用意するのが手っ取り早いと思います。

@Yosshi999
Copy link
Contributor Author

あれーテストが通らない

@PickledChair
Copy link
Member

Ubuntu と Mac に関しては、Python3 のコードを Python 2.7 で走らせていて、SyntaxError で落ちているようです。engine の方のテストやビルドは Python のバージョンを 3.8 に指定して走らせているので、参考になるかもしれません。

https://github.com/VOICEVOX/voicevox_engine/blob/a451258c937113e4d244a643cbaea9d86b1fdcc1/.github/workflows/test.yml
https://github.com/VOICEVOX/voicevox_engine/blob/a451258c937113e4d244a643cbaea9d86b1fdcc1/.github/workflows/build.yml

@Yosshi999
Copy link
Contributor Author

Yosshi999 commented Dec 16, 2021

x86のpythonモジュールのビルドが上手くいきませんね
__imp_* (x64の命名?)のリンクに失敗してるのでbuild_extでx64のコンパイラが走っている気がするのですがよく分かりません

@Hiroshiba
Copy link
Member

テスト結果はこちらで見れそうでしょうか
https://github.com/Yosshi999/voicevox_core/actions

なるほど・・・ちょっとわかりませんね・・・
最悪、windowsの32bitを切っちゃうという手もあるのかなとちょっと思いました。

Copy link
Member

@Oyaki122 Oyaki122 left a comment

Choose a reason for hiding this comment

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

build.ymlの294行目付近を

      # Build
      - if: startsWith(matrix.os, 'windows')
        uses: ilammy/msvc-dev-cmd@v1
        with: 
          arch: ${{ matrix.python_architecture }}

とすると通りました

@Yosshi999
Copy link
Contributor Author

全部通りました!ありがとうございます 🙇

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!
とても良いと思います!!!

Copy link
Member

@Oyaki122 Oyaki122 left a comment

Choose a reason for hiding this comment

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

LGTM!
お疲れさまでした

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.

ビルド手順の再考
4 participants