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

Allow to push resources after query_extra_data #4017

Merged
merged 12 commits into from
Apr 30, 2019
Merged

Allow to push resources after query_extra_data #4017

merged 12 commits into from
Apr 30, 2019

Conversation

badb
Copy link
Contributor

@badb badb commented Mar 18, 2019

We want to make sure that user is able to push additional resource for new subtasks. That can be useful if there is a task that may have dynamic resources that cannot be all defined and pushed during task creation (for example they are based on previous subtasks results).

Previously resources were only send during task creation and it was not possible to add them later.

@ghost ghost assigned badb Mar 18, 2019
@ghost ghost added the in progress label Mar 18, 2019
@badb badb changed the title [WIP] Allow to push resources after query_extra_data Allow to push resources after query_extra_data Apr 8, 2019
@@ -277,10 +276,35 @@ def _get_mask_for_task(client, task: coretask.CoreTask) -> masking.Mask:


@defer.inlineCallbacks
def _inform_subsystems(client, task, packager_result):
task_id = task.header.task_id
def add_resources(client, resources, res_id, timeout):
Copy link
Contributor

@mplebanski mplebanski Apr 9, 2019

Choose a reason for hiding this comment

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

Consider passing a resource_server directly, since you are never using client, but only it's field.

client_options=client_options,
)

logger.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this log message. What subsystems are we informing here and where do we do that?

common.deadline_to_timeout(ctd["deadline"])
)
resources = self.task_server.get_resources(ctd['subtask_id'])
ctd["resources"] = resources
Copy link
Contributor

Choose a reason for hiding this comment

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

Why resources from task_server are reassigned to CTD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because previously they were in the form that the app may give (just paths to files) and we want to change them to the one that were serialized properly by resource_manage (to_wire function). Provider expects to get resources in the serialized form (so from_wire function may be applied).

Copy link
Contributor

@mfranciszkiewicz mfranciszkiewicz Apr 19, 2019

Choose a reason for hiding this comment

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

Could you please add a brief comment?

@mplebanski
Copy link
Contributor

I rebased on my glambda0.3 branch to run my integration test with thin client. They work so it's rock solid change. Except from what I mentioned in the comments it looks good to me.

@mplebanski
Copy link
Contributor

Your PR will need to align with https://github.com/golemfactory/golem/issues/2223 Use MQ in offer_chosen callback pretty soon.

@ghost ghost added the in progress label Apr 16, 2019
@badb badb force-pushed the push_resource branch 2 times, most recently from 86ae134 to 653740e Compare April 17, 2019 13:32
@badb
Copy link
Contributor Author

badb commented Apr 17, 2019

For QA team:

I need you to focus on testing typical computation scenarios. Especially the one with:

  • multiple structure resource (ie. classroom)
  • many subtasks
  • some simple golem restarting scenarios (turning off and on during computation).

No specific and modified version of Electron is needed

@ZmijaWA @ederenn

@badb
Copy link
Contributor Author

badb commented Apr 25, 2019

@ZmijaWA @ederenn Branch updated.

@ZmijaWA
Copy link
Contributor

ZmijaWA commented Apr 26, 2019

@badb the task is under "testing" status ;)

@badb badb removed the in progress label Apr 29, 2019
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.

looks okay as far as I could tell :)

@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #4017 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4017      +/-   ##
===========================================
+ Coverage    88.79%   88.82%   +0.02%     
===========================================
  Files          215      215              
  Lines        18675    18690      +15     
===========================================
+ Hits         16583    16601      +18     
+ Misses        2092     2089       -3

@badb badb merged commit 6dd11f5 into develop Apr 30, 2019
@ghost ghost removed the needs review label Apr 30, 2019
@badb badb deleted the push_resource branch April 30, 2019 14:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants