-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Counter for number of improvements done #93
Counter for number of improvements done #93
Conversation
…o improvements-counter
…o improvements-counter
…o improvements-counter
- Changed a missed default value - Changed variable name
squash your changes and address conflicts when you feel ready |
@@ -55,6 +57,8 @@ def run(): | |||
args = parser.parse_args() | |||
opts.read(args) | |||
|
|||
improve_count = Value('i', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a good reason to make this a global variable. Instead, could high_level_interface.py
aggregate the total improvement count as improvements come in?
I think that design would also support the use case in #99: high_level_interface.py
could simply kill the worker processes when the number of improvements tips a threshold. This means the workers would need less internal logic, there is less global state and synchronization, and there would be one fewer parameters that need to pass between modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I realize this PR has already been merged, but it slipped under my radar. I think a simpler design here will make it much easier to build other features that use the improvement count in interesting ways.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that this the counter is in main.py
is because I wanted to have an easy way to print out the output of the counter after Cozy finishes running. Would putting it in high_level_interface.py
still be okay for that purpose (maybe with an import
)?
Also if you look here, I've changed the implementation to use a Queue
instead, so that I could also get the difference in time between each improve
call. Would putting it one level down in high_level_interface.py
add some difficulty to this as well?
If the changes are insignificant/easy to do, then I can make the change and create a new merge request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end of the day this is all a matter of taste and you should do what you feel is best. But just to clarify my thoughts:
If main.py
needs the count, then it is cleaner to have improve_implementation
return it (along with the improved AST that it already returns). Python makes it easy to return multiple values; using "out-parameters" is a dated and convoluted pattern. (Er, ok fine, some parts of the Cozy codebase already do it. But I promise the reason is to avoid unnecessary allocations and that it actually makes a difference in those places!)
The changes I would suggest are a little involved, but they feel like better design to me:
- remove the
improve_count
parameter from everywhere improve_implementation
should return a tuple of(ast, stats)
wherestats
is some documented structure that captures statistics about what happened during synthesis (like when different improvements happened)core.improve
can act the way it always hasimprove_implementation
can count the number of results it gets fromcore.improve
in this loop.- if you want to count restarts as well as actual improvements, modify the API of
core.improve
to yield, say,(result_type, result)
tuples, e.g.("improvement", x+1)
and("restart", some_new_example)
I like that design better since
- it avoids out-parameters
- it avoids shared mutable state
- it does not "leak" the use of
multiprocessing
to callers of theimprove_implementation
function - it does not require callers to deal with
multiprocessing
's weird caveats - it makes it easier for us to move away from
multiprocessing
someday (Which I have always dreamed about. Can you imagine distributed Cozy running on a cluster?) - it preserves the intended responsibilities of each module:
core.improve
finds improvements in a loop forever, andhigh_level_interface.improve_implementation
managescore.improve
threads by aggregating their results and deciding when to terminate them - it only adds one new responsibility to
improve_implementation
: aggregate statistics about thecore.improve
threads - it still allows us to use the statistics to inform the termination conditions in
improve_implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into this once the week starts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Calvin-L I agree with what you proposed, but I am concerned about how to stop the improve
loop if out-of-quota with this map-reduce like design, as what we are trying to achieve in #99.
P.S. I just made a clarified graph of Cozy call-chain for reference here:
main.run ->
synthesis.improve_implementation ->
Process.start ->
ImproveQueryJob.run ->
core.improve ->
while True: ... improve_count += 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened up a new issue for this discussion: #102 so we can keep track of it. Thanks for making this happen @Calvin-L @anhnamtran
This counter is added to aid with evaluating the rate at which improvements are
being made to implementations. This might be useful for determining speed-up improvements for Cozy.
Right now it is incremented every time step #1 of the
improve
loop finishes.