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

separate modules #3

Merged
merged 18 commits into from
Jul 3, 2023
Merged

separate modules #3

merged 18 commits into from
Jul 3, 2023

Conversation

ReiHashimoto
Copy link

@ReiHashimoto ReiHashimoto commented Jun 26, 2023

変更点

バックエンドで以下の調整を実施

  • configファイルの配置変更
    • optinist/api/config, condaのyamlファイルをwrappers/xxx/params, envに変更
      • snakemake, nwbのyamlはapi/snakemake, nwbに移動
    • wrappers/xxx_wrapperをwrappers/xxxに変更
    • 関数名からこれらのファイルパスを取得するため、filepath_finderを作成
    • conda_env, conda_nameを分ける必要性がないため(結局はconda/envs/に関数名でconda環境を作成しているかどうかによる)、wrapper_dictの内容をconda_nameのみに変更
  • 不要ファイルの整理
    • suite2p.yaml
      • 対応するfunctionがない
      • formatも無効な状態になっている
    • app_config/logging.yaml
      • ファイルにするほどの情報がない
    • pytest.ini
      • pyproject.tomlに統合
    • wrappers/suite2p/edit_roi
      • api/edit_ROI/wrappers/suite2p_edit_roiに移行した時の消し忘れ
    • api/dataclass/dataclass.py
      • 冗長なので__init__.pyに内容を移行
    • routers/fileIO
      • file_reader.pyをapi/utilsへ移動
    • wrappers/optinist_exception.py
      • 使用されていないファイル
  • api --> coreへrename
  • optinist --> studioへrename
    • 基本的にパスとしてのoptinistのみ
  • 区分の整理
    • routersはrouterだけにする
      • studio/routers/model.py --> schemasとしてapi単位に分離(後述)
      • studio/routers/const.py --> studio/appへ移動
    • config_reader writer, pickle_reader writerをutilsへ移動、また、xxx_handlerとしてファイルを集約
      • experimentなどと比べた際に、これらはdataclassを持たないため、utilsとして扱う
  • リファクタ
    • workflow_result.py
      • outputPaths: BaseDataのpropertyにすることで冗長さを回避
    • dir_path.py
      • 変数名を調整
    • rules/const.py
      • 変数名を調整
    • schemas
      • models.pyをschemasとしてrouterごとに分解
      • FILETYPEをconst.pyへ(schemaというよりは定数に近い内容のため)
  • docker-compose
    • 開発時にdocker run --> docker composeを使用するように修正
      • ソースをmountしているため、ローカルの変更が再ビルドせずに反映される
    • test, dev, prod用を作成
    • Dockerfile-xxxはDockerfile.xxxにrename(IDEがDockerfileであると識別できるようになるため)
  • core, routers, dataclass, wrappersをドメイン別に分離(common, optinist)
    • 現状はoptinist関連の内容を細分化できる程パターンがないため、この粒度で分離

注意

  • 下位互換性が失われる
    • 正確には、過去のworkflowのreproduceが不可
      • pickleファイルに保持されているimportのパスが変わってしまうため

@ReiHashimoto ReiHashimoto marked this pull request as draft June 26, 2023 06:56
@ReiHashimoto
Copy link
Author

以下2つのファイルはcommonだがcalcium_imagingからのimportを要してしまうため、要検討

  • studio/common/services/rules/file_writer.py
  • studio/common/services/rules/runner.py

※nwbをcalcium_imagingに固有と捉えるか、commonと捉えるかにも依存

@emuemuJP
Copy link
Collaborator

そうですね、あまり多くを考えても仕方ないのですがCalcium Imagingは一つの測定手法です
他にもLFPなど他の手法があり、これらをまとめるとNeuro Imagingということになります。
一方で、NeuroImagingにはMRI(磁気共鳴画像法)、PET(陽電子放出断層法)、fMRI(機能的磁気共鳴画像法)、CT(コンピュータ断層撮影)、EEG(脳波)、MEG(脳磁図)などたくさんあります。
なので、解析する対象の情報でわけるのはどうでしょうか?
NeuroImaging

  • 2D
  • 3D

@itutu-tienday
Copy link
Collaborator

@ReiHashimoto

以下幾つか、コメント記載します。
※コードの中身のレビューまでは行えていませんが、主にフォルダ・ファイル構造について、コメントします。

指摘事項

  • wrappers/xxx/env/ 内のファイルは、「condaのenv用である」ことが、フォルダ名・ファイル名から判別しやすいと、よい様に感じます。

    • → ファイル拡張子に .param や .conda を付与する対応は、適用しておいてもよいかもしれません。
  • optisnit/Snakemake ファイルは、この場所でよさそうでしょうか?

    • ※ config/ 配下などへ移動する形が適当?

指摘事項(優先度の低いもの)

  • アプリケーションのコンフィグフォルダについて

    app_config/logging.yaml
    ファイルにするほどの情報がない

    • 現状、上記の削除対応は問題ないと思いますが、アプリケーションのコンフィグフォルダ自体は用意があると、今後よいだろうと思われます。
      • → 現状は .env がアプリケーションのコンフィグファイルを兼ねていますが、本来アプリケーションコンフィグは 環境変数ではなく、専用の設定ファイルで管理する形式が適切と考えられる。
  • ROOT直下の notebooks/ は、このままでOK?

    • examples/ などの配下に格納してもよい?

@ReiHashimoto
Copy link
Author

ReiHashimoto commented Jun 29, 2023

@itutu-tienday
確認ありがとうございます!

  • wrappers/xxx/env/ 内のファイルは、「condaのenv用である」ことが、フォルダ名・ファイル名から判別しやすいと、よい様に感じます。
    • → ファイル拡張子に .param や .conda を付与する対応は、適用しておいてもよいかもしれません。

既にフォルダは分けていることもあり、.paramなどを追加要求とするのはやや冗長かもしれないと思っています。
一方で、envは確かに名前のスコープが広いので、condaenv, condaなどにrenameするのは賛成です。

  • optisnit/Snakemake ファイルは、この場所でよさそうでしょうか?
    • ※ config/ 配下などへ移動する形が適当?

Snakefileについては、snakemakeからの実行ファイルなのでconfigとはやや位置付けが異なりそうです。
また、edit_roiのようにドメイン別の分岐が入ってくる場合があるので、commonより上の階層に配置したいです。
移動するのであれば、studio/app配下かなと思っています。

  • アプリケーションのコンフィグフォルダについて

    app_config/logging.yaml
    ファイルにするほどの情報がない

  • 現状、上記の削除対応は問題ないと思いますが、アプリケーションのコンフィグフォルダ自体は用意があると、今後よいだろうと思われます。
  • → 現状は .env がアプリケーションのコンフィグファイルを兼ねていますが、本来アプリケーションコンフィグは 環境変数ではなく、専用の設定ファイルで管理する形式が適切と考えられる。

同意です。
そのため、studio/configディレクトリを設けました。
現在はdockerの設定のみですが、今後ここに他のconfigがあれば入れる想定です。

  • ROOT直下の notebooks/ は、このままでOK?
    • examples/ などの配下に格納してもよい?

ここは私も判断つかずでした、できれば移動したいですね。
sample_data/配下などでしょうか。
参考までに、CaImAnなどのリポジトリでは、demosなどの配下に入れてますね。

@itutu-tienday
Copy link
Collaborator

@ReiHashimoto

一方で、envは確かに名前のスコープが広いので、condaenv, condaなどにrenameするのは賛成です。

特に違和感なければ、env/ → conda/ でよいかなと思います。

移動するのであれば、studio/app配下かなと思っています。

上記でよさそうですね。

そのため、studio/configディレクトリを設けました。

了解しました、上記でOKですね。

sample_data/配下などでしょうか。

対象のファイルのカテゴリは、「introduction」などと思われるので、必ずしもデータでもないかも…?
→ ちょっと曖昧かもしれませんが、contents/ などを作成して格納するなど?

@ReiHashimoto
Copy link
Author

@itutu-tienday
以下2点は修正しました!

  • env --> condaにrename
  • Snakefileをstudio/app配下に移動

notebookは他にこのディレクトリにまとめたいものが出てきたタイミングで対応しましょうか

@ReiHashimoto ReiHashimoto marked this pull request as ready for review June 29, 2023 09:55
@itutu-tienday
Copy link
Collaborator

@ReiHashimoto
了解しました。 上記でOKかと思います。

現時点では、こちらからの指摘は一旦以上になります。

@ReiHashimoto ReiHashimoto merged commit 0328556 into develop-main Jul 3, 2023
3 checks passed
@ReiHashimoto ReiHashimoto deleted the feature/separate-modules branch July 3, 2023 01:28
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