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

[No.203]Workflow を新しいIDでcopy する機能 #496

Open
wants to merge 20 commits into
base: develop-main
Choose a base branch
from

Conversation

tsuchiyama-araya
Copy link
Collaborator

@tsuchiyama-araya tsuchiyama-araya commented Dec 3, 2024

【内容】
実装画面 → RECORD画面

「RUN ALL」を実行すれば同じワークフローを作成できることは理解しているが、再実行が必要なため時間がかかる。
そのため、実行状態を保持したまま、新しいIDや名前でワークフローをコピーできる機能が欲しい。
Reproduce機能の一つのオプションとして実装するのが適切だと思われる。
→ 右上のDeleteボタンの横に「Duplicate」ボタンを追加する。

Evidence
Screenshot 2024-12-17 at 11 23 08

@tsuchiyama-araya tsuchiyama-araya added the enhancement New feature or request label Dec 3, 2024
@tsuchiyama-araya tsuchiyama-araya self-assigned this Dec 3, 2024
@tsuchiyama-araya tsuchiyama-araya linked an issue Dec 3, 2024 that may be closed by this pull request
9 tasks
frontend/src/store/slice/Experiments/ExperimentsSlice.ts Outdated Show resolved Hide resolved
studio/app/common/core/experiment/experiment_writer.py Outdated Show resolved Hide resolved
studio/app/common/core/experiment/experiment_writer.py Outdated Show resolved Hide resolved
with open(expt_filepath, "r") as f:
config = yaml.safe_load(f)

config["unique_id"] = new_unique_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

更新箇所はここだけでよい?
→ experiment.yaml の中を確認すると、IDの更新漏れはないか?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

私の認識だと、こちらだけで大丈夫かと思います。
一応Outputフォルダーの中にunique_idで検索すると、Experiment Yamlのみで記載されています。

Copy link
Collaborator

Choose a reason for hiding this comment

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

Evidence(更新されたexperiment.yamlの内容)を添付していただけますか?
※こちらの確認では、上で記載のように、unique_id の更新漏れがあるように見えます。
※もし不具合があった場合、テストケースにこの不具合を検知するためのテストケースを追加しておいてください。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Evidenceを添付しました。
お手数ですが、ご確認お願い致します。

studio/app/common/routers/experiment.py Outdated Show resolved Hide resolved
studio/app/common/schemas/experiment.py Show resolved Hide resolved
studio/app/common/routers/experiment.py Outdated Show resolved Hide resolved
@tsuchiyama-araya
Copy link
Collaborator Author

@itutu-tienday
ご指摘いただき、ありがとうございました。
ご指摘いただいた箇所を修正いたしましたので、
お手数をおかけしますが、再度ご確認いただけますと幸いです。

studio/app/common/core/experiment/experiment_writer.py Outdated Show resolved Hide resolved
frontend/src/store/slice/Experiments/ExperimentsSlice.ts Outdated Show resolved Hide resolved
with open(expt_filepath, "r") as f:
config = yaml.safe_load(f)

config["unique_id"] = new_unique_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Evidence(更新されたexperiment.yamlの内容)を添付していただけますか?
※こちらの確認では、上で記載のように、unique_id の更新漏れがあるように見えます。
※もし不具合があった場合、テストケースにこの不具合を検知するためのテストケースを追加しておいてください。

@tsuchiyama-araya
Copy link
Collaborator Author

Evidence追記
Screenshot 2024-12-17 at 15 17 51
Screenshot 2024-12-17 at 15 16 40

logger = AppLogger.get_logger()

try:
for root, _, files in os.walk(directory):
Copy link
Collaborator

Choose a reason for hiding this comment

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

os.walk の利用は冗長と見受けられる。
処理対象となるファイルが自明なのであれば、glob などで明確に指定すべき。(仕様が明瞭にもなる)

) as file:
content = file.read()

if old_unique_id in content:
Copy link
Collaborator

Choose a reason for hiding this comment

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

この判定はやや危険がある可能性あり。(想定しないマッチパターンも拾う可能性)
判定条件の精度を高めた方が良い。
→ どの位置に unique_id が含まれ得るか、仕様を整理の上、その条件で正規表現で指定し置換する形がベター。

Copy link
Collaborator

Choose a reason for hiding this comment

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

こちらで適当な experiment の directory を id で grep してみたところ、pkl ファイルなどの中にも、idが含まれているようであることを確認。

※このケースは、想定されていたでしょうか?
※この状況を踏まえると、もしかすると、「1.copy」「2.copy元をdelete」「3.copy先を操作(workflow runなど)」の操作を行うと、pkl内のファイルパスがリンク切れとなり、エラーが生じるようなことは想定されないでしょうか?

$ grep -inrl "834359c6" * 
experiment.yaml
snakemake.yaml
suite2p_file_convert_sybncuz8n1/suite2p_file_convert.pkl
suite2p_file_convert_sybncuz8n1/suite2p/plane0/ops.npy
suite2p_registration_obwi55dmml/suite2p_registration.pkl
suite2p_roi_wavpgxg6co/suite2p_roi.pkl

frontend/src/store/slice/Experiments/ExperimentsSlice.ts Outdated Show resolved Hide resolved
@tsuchiyama-araya
Copy link
Collaborator Author

@itutu-tienday
お疲れ様です。
指摘した内容を修正しました。お手数ですが、再度ご確認お願い致します。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[No.203]Workflow を新しいIDでcopy する機能
3 participants