Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Remove resource size from task header #3917

Merged
merged 9 commits into from
Mar 1, 2019
Merged

Remove resource size from task header #3917

merged 9 commits into from
Mar 1, 2019

Conversation

badb
Copy link
Contributor

@badb badb commented Feb 21, 2019 •
edited
Loading

If we want to make possible to add different resources for different subtasks we cannot send just one size in the TaskHeader. This PR:

  • Removes resource_size from TaskHeader
  • Removes providers check of resource size at the task selection state
  • Provider may refuse to compute tasks with too big resources after getting TaskToCompute with specific resource size.
  • Taskkeeper keeping time is updated after TaskToCompute with proper resource size is sent.

Golem-messages PR: golemfactory/golem-messages#319

@ghost ghost assigned badb Feb 21, 2019
@ghost ghost added the in progress label Feb 21, 2019
@badb badb changed the title Remove resource size from task header [WIP] Remove resource size from task header Feb 21, 2019
@badb badb force-pushed the task_header_cleanup branch 2 times, most recently from 12a1911 to 817eeba Compare February 24, 2019 15:48
@badb badb force-pushed the task_header_cleanup branch 2 times, most recently from 1d5032c to 1a83c28 Compare February 24, 2019 16:58
@badb badb force-pushed the task_header_cleanup branch from 1a83c28 to 64d4d2a Compare February 24, 2019 17:05
@badb badb force-pushed the task_header_cleanup branch from 0f1de69 to 1fe89b7 Compare February 24, 2019 20:13
@badb badb mentioned this pull request Feb 24, 2019
Merged
@badb badb requested review from jiivan and shadeofblue February 24, 2019 20:33
shadeofblue
shadeofblue reviewed Feb 27, 2019
View reviewed changes
golem/task/server/resources.py
if task_header:
options.set(size=task_header.resource_size)
if size and options:
options.set(size=size)
Copy link
Contributor

Choose a reason for hiding this comment

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

@badb there seems to be something not quite right here... unless I'm misreading...

what I mean is that previously, the size here had always been set to the resource size specified in the task header...

right now, the only call to this method that specifies the size, uses the ReportComputedTask.size which is not the resource size but rather the result size ... is that indeed what we need here? am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Size option can be also already specified in options (that should be the case for TaskToCompute (check line 233 in taskkeeper).
This option is used to estimate how long may it take to download resources. With ReportComputedTask we want to dowload results so their size should be used. I guess that usage of resource_size in this place was just a heuristic (we assumed that results size won't be significantly different that size of resources, which of course doesn't have to be true).

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

shadeofblue
shadeofblue approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

girl a okay stockphoto

@codecov
Copy link

codecov bot commented Feb 27, 2019 •
edited
Loading

Codecov Report

Merging #3917 into develop will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop   #3917      +/-   ##
==========================================
+ Coverage    87.66%   87.8%   +0.13%     
==========================================
  Files          216     216              
  Lines        18968   18977       +9     
==========================================
+ Hits         16629   16662      +33     
+ Misses        2339    2315      -24

@ghost ghost added the in progress label Feb 27, 2019
@badb badb changed the title [WIP] Remove resource size from task header Remove resource size from task header Feb 28, 2019
@badb badb merged commit 68d07b4 into develop Mar 1, 2019
@ghost ghost removed the in progress label Mar 1, 2019
@badb badb deleted the task_header_cleanup branch March 1, 2019 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shadeofblue shadeofblue shadeofblue approved these changes

@jiivan jiivan Awaiting requested review from jiivan

Assignees

@badb badb

Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@badb @shadeofblue