Repository navigation
Update [Notebook] : Add bypass function to directly download outputsfrom execution name + add check on each step to verify needed files are here - #59
Conversation
…from execution name + add check on each step to verify needed files are here
…cks, clean diffs and idempotence to freesurfer notebook
|
|
||
| dest = Path(where_to_save) | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| with tempfile.NamedTemporaryFile(dir=dest.parent, prefix=f".{dest.name}.", suffix=".part", delete=False) as tmp: |
There was a problem hiding this comment.
why not having delete=True and rely on it to delete it anyway and avoid the finally block ?
There was a problem hiding this comment.
Since we move/replace the file downloaded, delete=True would try to delete a non-existent file and this may cause errors on windows.
I made a mistake here (fixed), the os.replace line should be outside the with, as on windows trying to move an open file can cause errors. So we definitely need the file to not be deleted after the with, but there may be better ways to do it 🤔
| try: | ||
| os.remove(tmp_path) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
it would be nice to reuse the code from the other method
There was a problem hiding this comment.
Indeed, created a download_file generic helper function
| "# Create LOCAL_DATASET_PATH, derivatives and freesurfer folders if they don't exist\n", | ||
| "FREESURFER_DIR.mkdir(parents=True, exist_ok=True)\n", | ||
| "# Download dataset from Girder\n", | ||
| "cfg.GIRDER_CLIENT.downloadFolderRecursive(cfg.GIRDER_DATASET_PATH, str(FS_CROSS_INPUT_DIR))\n", |
There was a problem hiding this comment.
as it downloads in FS_CROSS_INPUT_DIR, should not we create this directory in the mkdir just above instead of FREESURFER_DIR ?
There was a problem hiding this comment.
downloadFolderRecursive from Girder creates the missing parent directories on its own, but I still added it as its cheap and clearer. The opposite would have been to remove all mkdirs (even FREESURFER_DIR would have been created by downloadFolderRecursive.
| " if f.is_dir():\n", | ||
| " flatten_folder(Path(f))" | ||
| "session_name = restore_session_from_output_dir(FS_CROSS_OUTPUT_DIR) # Replace with a specific session name if needed\n", | ||
| "download_by_session_name(session_name, str(FS_CROSS_OUTPUT_DIR))\n", |
There was a problem hiding this comment.
is it to avoid downloading files already present ?
does not session.download_outputs handle that ?
There was a problem hiding this comment.
Yes, it avoid re-downloading by checking existence and file size. It also avoids the creation of the VIP timestamp folder that I had to flatten before.
session.download_outputs does try to check for existing files but include the timestamp folder in the path, which I don't have if it's a rerun (flattened). Other than that it has the download size check I added in the new download functions, but in my download_by_session_name function I also do the VIP expected size check pre-download to filter files.
| "for f in FS_CROSS_OUTPUT_DIR.iterdir():\n", | ||
| " if f.is_dir():\n", | ||
| " flatten_folder(Path(f))" | ||
| "session_name = restore_session_from_output_dir(FS_CROSS_OUTPUT_DIR) # Replace with a specific session name if needed\n", |
There was a problem hiding this comment.
can there be several session_name in 1 output folder ?
There was a problem hiding this comment.
No there can be only one, as there is only one session_data.json per output folder
| "# Since results may arrive in a VipSession folder (2026-X), when it's the case we flatten this folder to put files directly under FS_CROSS_OUTPUT_DIR\n", | ||
| "for f in FS_CROSS_OUTPUT_DIR.iterdir():\n", | ||
| " if f.is_dir():\n", | ||
| " flatten_folder(Path(f))" |
There was a problem hiding this comment.
i think it is better to not do that, but does it not change something when we when to list the tar.gz latter ?
Or does the new download_by_session_name method change that ?
There was a problem hiding this comment.
Was removed by previous commit since we now directly download files inside output folder and not through VIP timestamp folder, so we don't need the flatten anymore.
For the tar.gz we need the files to be directly under the output folder to use FS_CROSS_OUTPUT_DIR.iterdir().
| " out_tar = FS_BASE_INPUT_DIR / f\"{subj}_TPs.tgz\"\n", | ||
| " if out_tar.exists() and out_tar.stat().st_size > 0:\n", | ||
| " print(f\" → {out_tar.name} already exists, skipping.\")\n", | ||
| " continue\n", |
There was a problem hiding this comment.
could it be possible that the existing tar is present but incomplete because a data was missing ?
There was a problem hiding this comment.
It could be possible, I added checks for missing files, corruption, and re-extracting when base inputs are newer
3 participants
No description provided.