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

Fix config cannot be updated #5171

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Fix config cannot be updated #5171

merged 2 commits into from
Apr 27, 2020

Conversation

etam
Copy link
Contributor

@etam etam commented Apr 24, 2020

Fixes #5151

@etam etam self-assigned this Apr 24, 2020
@etam etam force-pushed the fix-config-cannot-be-updated branch from 99becef to d583b8a Compare April 24, 2020 12:23
Copy link

@Wiezzel Wiezzel left a comment

Choose a reason for hiding this comment

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

I have worries about potential race conditions occurring when the config is updated at the very moment when environment gets enabled/disabled. Environment state could change between status check in update_config and calling super().update_config or assigning _next_config.

@Wiezzel
Copy link

Wiezzel commented Apr 24, 2020

And you're gonna have a hard time silencing mypy when dealing with class decorators...

@etam etam force-pushed the fix-config-cannot-be-updated branch 2 times, most recently from f440c41 to 53ff023 Compare April 24, 2020 15:43
@etam
Copy link
Contributor Author

etam commented Apr 24, 2020

I have worries about potential race conditions occurring when the config is updated at the very moment when environment gets enabled/disabled. Environment state could change between status check in update_config and calling super().update_config or assigning _next_config.

How about now?

And you're gonna have a hard time silencing mypy when dealing with class decorators...

It wasn't that bad :)

Copy link

@Wiezzel Wiezzel left a comment

Choose a reason for hiding this comment

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

The lock seems to do the job.
One more small comment.

golem/envs/__init__.py Outdated Show resolved Hide resolved
wait_for_computing_task,
partial(ui_stop, node_id=NodeId.provider),
partial(change_config, node_id=NodeId.provider),
partial(check_if_test_failed, node_id=NodeId.provider),
Copy link
Collaborator

@shadeofblue shadeofblue Apr 27, 2020

Choose a reason for hiding this comment

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

shouldn't we somehow detect that the configuration has been completed? (instead of waiting the 10 seconds)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we should, but in the current "fire and forget" implementation model, it's not easy to achieve. I tried.

Copy link
Collaborator

@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 good as far as I could tell, added a couple of comments

golem/task/taskcomputer.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 27, 2020

Codecov Report

Merging #5171 into b0.23 will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            b0.23    #5171      +/-   ##
==========================================
+ Coverage   89.93%   89.95%   +0.01%     
==========================================
  Files         238      238              
  Lines       22460    22482      +22     
==========================================
+ Hits        20200    20224      +24     
+ Misses       2260     2258       -2     

@etam etam force-pushed the fix-config-cannot-be-updated branch from 742fe9c to cb62998 Compare April 27, 2020 11:13
@etam etam requested a review from Wiezzel April 27, 2020 11:16
@etam etam merged commit cda6a9f into b0.23 Apr 27, 2020
@etam etam deleted the fix-config-cannot-be-updated branch April 27, 2020 11:48
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.

3 participants