Skip to content

Conversation

@keigohtr
Copy link
Member

What is this PR for?

When we switch model, it does not work.
This is because the current code updates a local variable. So I changed to use a list object to update predictor object.

This PR includes

  • Use list as input

What type of PR is it?

Bugfix

What is the issue?

N/A

How should this be tested?

$ python -m unittest

@keigohtr keigohtr self-assigned this Mar 11, 2019
Copy link
Member

@yuki-mt yuki-mt left a comment

Choose a reason for hiding this comment

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

@keigohtr
I figured out what is wrong, but this solution (and current situation) has very implicit dependency between RekcurdDashboardServicer and RekcurdWorkerServicer, which may make it difficult to read the code.

What about adding setter of predictor to RekcurdWorkerServicer and call it in RekcurdDashboardServicer?

@keigohtr
Copy link
Member Author

@yuki-mt
Thank you for your review.
Hummmm... I think having an implicit dependency between RekcurdDashboardServicer and RekcurdWorkerServicer is natural for this case since we need to share predictor during the application.

And, adding one servicer to another is weird for me since it seems they are parent-child relationship. Using a pointer and sharing variables is not strange implementation I think.

But we can create a class and use it instead of list is a good idea I think.
How do you think?

@yuki-mt
Copy link
Member

yuki-mt commented Mar 12, 2019

adding one servicer to another is weird for me since it seems they are parent-child relationship.

DashbaordServicer affects WorkerServicer in fact, so this is not weird to me, but

I think having an implicit dependency between RekcurdDashboardServicer and RekcurdWorkerServicer is natural for this case since we need to share predictor during the application.

I agree with it.

But we can create a class and use it instead of list is a good idea I think.

I also agree with it. It looks better.

@keigohtr
Copy link
Member Author

@yuki-mt I have updated. Could you review this again please?

@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #43 into master will increase coverage by <.01%.
The diff coverage is 98.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   88.05%   88.06%   +<.01%     
==========================================
  Files          15       15              
  Lines         854      863       +9     
==========================================
+ Hits          752      760       +8     
- Misses        102      103       +1
Impacted Files Coverage Δ
rekcurd/core/__init__.py 100% <100%> (ø) ⬆️
rekcurd/core/rekcurd_worker_servicer.py 87.03% <100%> (-0.08%) ⬇️
rekcurd/core/rekcurd_dashboard_servicer.py 97.29% <100%> (+0.12%) ⬆️
rekcurd/core/rekcurd_worker.py 76.47% <80%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5795490...a5a342f. Read the comment docs.

Copy link
Member

@yuki-mt yuki-mt left a comment

Choose a reason for hiding this comment

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

LGTM

@keigohtr keigohtr merged commit d3f7c4d into master Mar 12, 2019
@keigohtr keigohtr deleted the fix/use-pointer-for-rekcurd branch March 12, 2019 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants