{"action":"created","issue":{"url":"https://api.github.com/repos/quiltdata/t4/issues/102","repository_url":"https://api.github.com/repos/quiltdata/t4","labels_url":"https://api.github.com/repos/quiltdata/t4/issues/102/labels{/name}","comments_url":"https://api.github.com/repos/quiltdata/t4/issues/102/comments","events_url":"https://api.github.com/repos/quiltdata/t4/issues/102/events","html_url":"https://github.com/quiltdata/t4/pull/102","id":378535350,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI5MjE5MDkw","number":102,"title":"threaded upload for _materialize(), and therefore push().","user":{"login":"eode","id":2475983,"node_id":"MDQ6VXNlcjI0NzU5ODM=","avatar_url":"https://avatars3.githubusercontent.com/u/2475983?v=4","gravatar_id":"","url":"https://api.github.com/users/eode","html_url":"https://github.com/eode","followers_url":"https://api.github.com/users/eode/followers","following_url":"https://api.github.com/users/eode/following{/other_user}","gists_url":"https://api.github.com/users/eode/gists{/gist_id}","starred_url":"https://api.github.com/users/eode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eode/subscriptions","organizations_url":"https://api.github.com/users/eode/orgs","repos_url":"https://api.github.com/users/eode/repos","events_url":"https://api.github.com/users/eode/events{/privacy}","received_events_url":"https://api.github.com/users/eode/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-11-08T01:04:08Z","updated_at":"2018-11-08T22:02:39Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/quiltdata/t4/pulls/102","html_url":"https://github.com/quiltdata/t4/pull/102","diff_url":"https://github.com/quiltdata/t4/pull/102.diff","patch_url":"https://github.com/quiltdata/t4/pull/102.patch"},"body":"This was pretty simple:\r\n\r\n* `concurrent.futures`\r\n* `ThreadPoolExecutor`, because a Process Pool would be way overkill, and it's network, not CPU bound\r\n* TQDM Just Magically Worked -- multiline display and all."},"comment":{"url":"https://api.github.com/repos/quiltdata/t4/issues/comments/437172259","html_url":"https://github.com/quiltdata/t4/pull/102#issuecomment-437172259","issue_url":"https://api.github.com/repos/quiltdata/t4/issues/102","id":437172259,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNzE3MjI1OQ==","user":{"login":"eode","id":2475983,"node_id":"MDQ6VXNlcjI0NzU5ODM=","avatar_url":"https://avatars3.githubusercontent.com/u/2475983?v=4","gravatar_id":"","url":"https://api.github.com/users/eode","html_url":"https://github.com/eode","followers_url":"https://api.github.com/users/eode/followers","following_url":"https://api.github.com/users/eode/following{/other_user}","gists_url":"https://api.github.com/users/eode/gists{/gist_id}","starred_url":"https://api.github.com/users/eode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eode/subscriptions","organizations_url":"https://api.github.com/users/eode/orgs","repos_url":"https://api.github.com/users/eode/repos","events_url":"https://api.github.com/users/eode/events{/privacy}","received_events_url":"https://api.github.com/users/eode/received_events","type":"User","site_admin":false},"created_at":"2018-11-08T22:02:39Z","updated_at":"2018-11-08T22:02:39Z","author_association":"CONTRIBUTOR","body":"Well, nested `ThreadPoolExecutor`s isn't really an issue, so long as it's not for something that leads to exponentially increasing thread count.  The basic situation is:\r\n\r\n* Is it worth it to parallelize `push()`? ..it seems so.\r\n* For any given push, is it worth it to parallelize etag iteration and object version listing? ..it seems so.\r\n\r\n..the result should, then, also be worth it -- to an extent.  Since we're basically focusing on overcoming io-bound limits, we don't need a ton of workers on the task, so long as for any given IO source, there's at least one item running.\r\n\r\nPotential Solutions:\r\n* Limit both `ThreadPoolExecutor`s `max_workers`\r\n* Limit `upload_file()` to max_workers=2, since it's primarily parallelizing reading files and network IO anyways\r\n* Pass a `_max_workers` param on down the line, from `pkg._materialize()` to `pkg._copy()` to `data_transfer.copy_file()` to  `data_transfer.upload_file()`\r\n  * 🤨\r\n* Keep some kind of global state\r\n  * 😾\r\n* Don't parallelize `Package.push()`\r\n\r\nRecommendation:\r\n* Limit both `ThreadPoolExecutor`s\r\n  * `upload_file()` to 1 or 2 additional threads, as it is primarily serializing two IO tasks, not CPU ones\r\n    * though hashing can be CPU intensive, threading doesn't help, as it's all on the same CPU core.\r\n  * `push` to 2 or 3, as it is primarily just trying to keep IO going while other network connections are being established\r\n\r\n@dimaryaz \r\nSince I'm poking around at other peoples' code, can I get an approval on that thought?  \r\n\r\nthanks..\r\n"}}
{"actor":{"display_login":"eode"}}