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

Allow multiple simultaneous uploads via single POST. #4563

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
4 participants
@jmchilton
Copy link
Member

jmchilton commented Sep 5, 2017

The upload.py tool itself already allowed this.

@guerler

This comment has been minimized.

Copy link
Contributor

guerler commented Sep 6, 2017

Is this ready for review and will this allow to specify separate dbkey/datatype pairs for each file?

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Sep 7, 2017

It lets you add different names, space_to_tab and to_posix_lines values, and such - but I guess not different dbkeys and filetypes. I'll update the PR to do this and add a test case for that this morning.

if override_file_type:
return override_file_type
else:
return context.get(self.file_type_name, self.default_file_type)

This comment has been minimized.

Copy link
@guerler

guerler Sep 7, 2017

Contributor

Very minor but could we do something like unless you think its less performant:

default_type = context.get(self.file_type_name, self.default_file_type)
return context.get("override_file_type", default_type)

I also wonder if we could just name it file_type instead of override_file_type since they can be distinguished by being specified on request vs file bunch level. Thanks a lot for this addition. This is awesome.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 7, 2017

Author Member

Yeah - so I had that at first but the fact that a value - even though it was empty - was in the parent child context means the parent context was ignored. I should probably take another swing at this though - because you are right - the way it is now kinda sucks.

@jmchilton jmchilton added the status/WIP label Sep 7, 2017

@@ -279,10 +285,18 @@ def value_from_basic(self, value, app, ignore_errors=False):
rval.append(rval_dict)
return rval

def get_file_count(self, trans, context):
file_count = context.get("file_count", "auto")
if file_count == "auto":

This comment has been minimized.

Copy link
@anuprulez

anuprulez Sep 15, 2017

Member

Just a minor one. Can we make it one liner?

file_count = len(self.get_datatype(trans, context).writable_files) if file_count == "auto" else int(file_count)

thank you!

This comment has been minimized.

Copy link
@jmchilton

jmchilton Oct 2, 2017

Author Member

I'll rebase with this change, thanks!

@jmchilton jmchilton force-pushed the jmchilton:multiple_files_per_upload branch from ddeb712 to e6fbd9b Oct 2, 2017

Allow multiple simulatenous uploads via single POST.
The upload.py tool itself already allowed this and update upload dataset grouping to handle this.

@jmchilton jmchilton force-pushed the jmchilton:multiple_files_per_upload branch from e6fbd9b to 1fa4ea2 Oct 2, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Oct 2, 2017

I've changed the interface here so that "file_type" or "dbkey" in the parent context are taken as defaults and in the "per-file" context these can be overridden with "file_type" or "dbkey" instead of requiring a different variable "override_file_type" / "override_dbkey" as I had it before. I added a few more tests for different combinations of override versus default for dbkey and file_type.

@jmchilton jmchilton added status/review and removed status/WIP labels Oct 2, 2017

@guerler

This comment has been minimized.

Copy link
Contributor

guerler commented Oct 3, 2017

Thanks a lot for working on this. I aligned the submission format in #4513 and it seems to work well.

@jmchilton jmchilton referenced this pull request Oct 3, 2017

Merged

Many more upload tests. #4746

if dbkey == "":
if parent_context:
dbkey = parent_context.get("dbkey", dbkey)
return dbkey

This comment has been minimized.

Copy link
@guerler

guerler Oct 4, 2017

Contributor

I might be missing something but why are we not just returning parent_context.get("dbkey", context.get("dbkey"))?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Oct 4, 2017

Author Member

So the inner dbkey - the per file one - is just a hidden field in the tool form with a default of "" - that is what the outer one is checking the top-level dbkey param in the tool that I think will default to ?. So if the API request sends a dbkey for all files and then one for a specific file - I think we should use the base for all files that don't specify an explicit dbkey and then use the specific keys as set. I tried to use this strategy with the file extensions also. This implementation is crap because we are mapping API requests to a tool that wasn't really ever designed to do this well (these frustrations made led me to create #4734).

So I implemented API tests to verify all this behavior with respect to specific file dbkey versus dbkey for all files - if you can rework the implementation to be cleaner in such a way that the API calls don't break - I'm totally on board. I'm quite frustrated with a lot of grouping.py. There are more test cases in #4746 that should also help verify cleanups to the implementation don't break FTP uploads and such.

@guerler guerler merged commit 79a16da into galaxyproject:dev Oct 4, 2017

6 checks passed

api test Build finished. 301 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Oct 4, 2017

Thanks for the merge @guerler - let me know if there is anything else I can do to help with optimizing uploads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.