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.

Sign up for GitHub

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

Jump to bottom

RE2022-209: workspace uploader (working script) #381

Merged
merged 97 commits into from
Aug 17, 2023
Merged

RE2022-209: workspace uploader (working script) #381

merged 97 commits into from
Aug 17, 2023

Conversation

Xiangs18
Copy link
Collaborator

Please note that unit tests will be in a separate PR

Xiangs18 and others added 30 commits June 29, 2023 17:10
@Xiangs18
test uploader
24ce4ad
@Xiangs18
update params
366376e
@Xiangs18
update assembly name
534d176
@Xiangs18
get_assembly_as_fasta before upload
fa9ee86
@Xiangs18
testing
ec3be6e
@Xiangs18
restructure
b822ef8
@Xiangs18
update workspace_name
201c0fb
@Xiangs18
add .fasta
b42ba13
@Xiangs18
test again
75c3caf
@Xiangs18
assert path exist
a87d171
@Xiangs18
workspace name
b68792c
@Xiangs18
modify file path
06af0e6
@Xiangs18
update path
b57806c
@Xiangs18
switch to .fna file extension
aa4a22a
@Xiangs18
remove underscore
54583d9
@Xiangs18
use the latest AssemblyUtilClient.py file
838530d
@Xiangs18
use the bulk method
2def748
@Xiangs18
add {}
dc2e39b
@Xiangs18
remove dict for path
f3961eb
@Xiangs18
Merge branch 'main' into dev-worksapce_uploader
0b67670
@Xiangs18
finish majority of script; need add vol and test
a6fa6e9
@Xiangs18
refactor for easier in parallel setup
9302c50
@Xiangs18
initial test for workspace_uploader
5c2bc13
@Xiangs18
update image name
b7a7b75
@Xiangs18
fix incorrect env for ncbi
6769b67
@Xiangs18
use Tuple[]
1b4ea90
@Xiangs18
mount work_dir
32e9bae
@Xiangs18
make message more descrptive
1996a05
@Xiangs18
give it the path from the mount point inside the container
5e84b3b
@Xiangs18
hard link files of interest and update script
514a609
MrCreosote
MrCreosote reviewed Aug 15, 2023
View reviewed changes
Copy link
Member

@MrCreosote MrCreosote left a comment

Choose a reason for hiding this comment

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

Just about done, just docs and minor fixes.

@Tianhao-Gu can you take a look as well?

This is a very large number of commits for what is ~70% green field code, so once everything is done and the reviewers are happy I'm thinking that this PR should be closed, and an equivalent PR opened with the same contents but just one commit

src/loaders/workspace_downloader/workspace_downloader_helper.py Outdated Show resolved Hide resolved
src/loaders/workspace_downloader/workspace_downloader_helper.py Outdated Show resolved Hide resolved
src/loaders/workspace_downloader/workspace_downloader_helper.py Outdated Show resolved Hide resolved
src/loaders/common/loader_helper.py Outdated Show resolved Hide resolved
src/loaders/workspace_downloader/workspace_downloader_helper.py Show resolved Hide resolved
@MrCreosote
Copy link
Member

This LGTM. @Tianhao-Gu if you approve as well, I'd suggest (like I said before) that this PR should be closed and a new one created with the same contents in a single commit

@Xiangs18
Copy link
Collaborator Author

Xiangs18 commented Aug 16, 2023 •
edited
Loading

Hi @MrCreosote, can we just do squash and merge? It will have the same contents in a single commit on main.

Tianhao-Gu
Tianhao-Gu previously approved these changes Aug 16, 2023
View reviewed changes
Copy link
Collaborator

@Tianhao-Gu Tianhao-Gu left a comment

Choose a reason for hiding this comment

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

just a minor change. Other than that looks good to me.
By the way, setting workers other than 5 works for you?

src/loaders/workspace_uploader/workspace_uploader.py Outdated Show resolved Hide resolved
MrCreosote
MrCreosote approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@MrCreosote MrCreosote left a comment

Choose a reason for hiding this comment

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

Once Tian approves go head and squash & merge for this one time

@Xiangs18
Copy link
Collaborator Author

@Tianhao-Gu, I just tried a bunch of stuff.

5(default workers count) seems like a magic number. num workers <= 5 works and anything > 5 gets stuck in the middle. This applies to both the uploader and downloader scripts.

I currently don't have a solution in mind. It might have something to do with the callback server. We need to consult Shane.

@MrCreosote
Copy link
Member

anything > 5 gets stuck in the middle

What actually happens?

Tianhao-Gu
Tianhao-Gu approved these changes Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@Tianhao-Gu Tianhao-Gu left a comment

Choose a reason for hiding this comment

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

PR looks good to me. We can merge it. workers > 5 is a separate issue. Maybe just create an issue or a ticket to remind us.

@Xiangs18 Xiangs18 merged commit a8b506d into main Aug 17, 2023
9 of 11 checks passed
@Xiangs18 Xiangs18 deleted the dev-worksapce_uploader branch August 17, 2023 17:24
@Xiangs18 Xiangs18 mentioned this pull request Aug 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tianhao-Gu Tianhao-Gu Tianhao-Gu approved these changes

@MrCreosote MrCreosote MrCreosote approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Xiangs18 @MrCreosote @Tianhao-Gu