[No.203]Workflow を新しいIDでcopy する機能 #496
Conversation
…o into develop-main
…o into develop-main
frontend/src/components/Workspace/Experiment/ExperimentTable.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/Workspace/Experiment/ExperimentTable.tsx
Outdated
Show resolved
Hide resolved
|
@itutu-tienday |
|
Evidence追記 |
| ) as file: | ||
| content = file.read() | ||
|
|
||
| if old_unique_id in content: |
There was a problem hiding this comment.
この判定はやや危険がある可能性あり。（想定しないマッチパターンも拾う可能性）
判定条件の精度を高めた方が良い。
→ どの位置に unique_id が含まれ得るか、仕様を整理の上、その条件で正規表現で指定し置換する形がベター。
There was a problem hiding this comment.
こちらで適当な 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
There was a problem hiding this comment.
この判定はやや危険がある可能性あり。（想定しないマッチパターンも拾う可能性）
→ どの位置に unique_id が含まれ得るか、仕様を整理の上、その条件で正規表現で指定し置換する形がベター。
まだ上記が対応されていない模様。（単純な str.replace が用いられている）
指摘の意図を踏まえ、改善策を適用してください。
|
@itutu-tienday |
| ) as file: | ||
| content = file.read() | ||
|
|
||
| if old_unique_id in content: |
There was a problem hiding this comment.
この判定はやや危険がある可能性あり。（想定しないマッチパターンも拾う可能性）
→ どの位置に unique_id が含まれ得るか、仕様を整理の上、その条件で正規表現で指定し置換する形がベター。
まだ上記が対応されていない模様。（単純な str.replace が用いられている）
指摘の意図を踏まえ、改善策を適用してください。
| logger.error(f"Error replacing unique_id in files: {e}") | ||
| return False | ||
|
|
||
| def __update_yaml(self, file_path: str, old_id: str, new_id: str) -> None: |
There was a problem hiding this comment.
関数名（__update_xxx）がやや単純であり、関数名から機能が把握しづらく見える。
用途（copy_data からしか参照されない関数でもある）を明確にした名称とする。
| except Exception as e: | ||
| logger.warning(f"Failed to update YAML {file_path}: {e}") | ||
|
|
||
| def __update_pickle(self, file_path: str, old_id: str, new_id: str) -> None: |
There was a problem hiding this comment.
他の `def __update_xxx' と同様、関数名を refactoring する
| except Exception as e: | ||
| logger.warning(f"Failed to update Pickle {file_path}: {e}") | ||
|
|
||
| def __update_npy(self, file_path: str, old_id: str, new_id: str) -> None: |
There was a problem hiding this comment.
他の `def __update_xxx' と同様、関数名を refactoring する
| except Exception as e: | ||
| logger.warning(f"Failed to update NPY {file_path}: {e}") | ||
|
|
||
| def __replace_ids_recursive( |
There was a problem hiding this comment.
この処理（def copy_data からの呼び出し）は、自動テスト（想定通りに各データが置換されているかどうか）を用意しておくのがよいと思われる。
※自動テストの実装作業は、簡易に対応可能であればこのPR内で、多少の時間を要する場合は、別issueで対応を想定してください。
| else: | ||
| return obj | ||
|
|
||
| def __update_experiment_config_name(self, output_dir: str) -> bool: |
There was a problem hiding this comment.
他の `def __update_xxx' と同様、関数名を refactoring する
|
|
||
| config["name"] = f"{config.get('name', 'experiment')}_copy" | ||
| with open(config_path, "w") as file: | ||
| yaml.safe_dump(config, file) |
There was a problem hiding this comment.
他の箇所のyaml保存では、yaml の dump には yaml.dump が使用されています。
yaml.safe_dump の方が本来適切そうですが、箇所により使用されるyaml出力関数が異なるのは、あまり適切ではなさそうです。
ここで yaml.safe_dump を利用している明確な意図はありますか？
特に明確な意図がなければ、他の箇所とコードを統一しておいてください。
|
|
||
| config["name"] = f"{config.get('name', 'experiment')}_copy" | ||
| with open(config_path, "w") as file: | ||
| yaml.safe_dump(config, file) |
There was a problem hiding this comment.
ここで yaml dump すると、experiment.yaml 内の構造（key の order）が全体的に変更されています。
以下を参考に、構造を維持する調整を適用してください。
-
また上記の仕様（yamlファイルの構造維持）を、テストケースにも追記しておいてください。
【内容】
実装画面 → RECORD画面
「RUN ALL」を実行すれば同じワークフローを作成できることは理解しているが、再実行が必要なため時間がかかる。
そのため、実行状態を保持したまま、新しいIDや名前でワークフローをコピーできる機能が欲しい。
Reproduce機能の一つのオプションとして実装するのが適切だと思われる。
→ 右上のDeleteボタンの横に「Duplicate」ボタンを追加する。
requirement
testcase
Evidence
