-
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
Feat/replay #108
Feat/replay #108
Conversation
bobbyyyan
commented
Feb 22, 2021
- fix no-op when no args passed
- add support for literals
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.
Hi Bobby, good catch on returning literal values. Let's set up a time to talk and go through design options before merging. I prefer for the developer to specify whether to return a value or update an argument's state.
flor.skipblock.end(args, values=[...]
Here, args is a tuple of objects whose states are updated.
Values is what we mean to return.
Example.
favorite_int = 99
...
favorite_int = flor.SkipBlock.end(net, optimizer, values=[favorite_int,])
We can discuss during our call. Thanks!!
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.
Thanks for your work implementing the logger Bobby. The logger is looking solid and it has a good interface. Thank you.
I would like for you to separate the ReadBlock refactor and feature extension from the logger implementation please. That will facilitate design, discussion, and debugging and testing. I didn't really read over the ReadBlock changes. Please submit a PR for the logger only ignoring ReadBlock modifications, then during our 1:1 we can discuss possible ReadBlock extensions as necessary.
Please also address comments about code deduplication.
Thanks again!
flor/journal/file.py
Outdated
|
||
import json | ||
import pathlib | ||
from typing import Union, List | ||
|
||
entries: List[Union[DataRef, DataVal, Bracket, EOF]] = [] | ||
|
||
def _proc_log_record(log_record): |
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.
Isn't this function already defined elsewhere in the code? Can we avoid code duplication here? Have the code block be defined in a single spot and used in multiple spots.
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 couldn't find where it's duplicated. Mind pointing me to it?
flor/journal/file.py
Outdated
return json.dumps(log_record.jsonify()) + pathlib.os.linesep | ||
|
||
entries_logger = Logger() | ||
entries_logger.register_pre(_proc_log_record) |
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 think I see what you're doing here. You're being clear about the code that gets run on pre
. But the user won't be changing the Flor code base, so this show-and-tell format is better off in a readme or Docs.
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'm not too sure what you mean by "show-and-tell format." It's registering a preprocessing step particular to entries_logger
but not all loggers.
For completeness, the argument against
is that it puts the burden on the user to determine what's pass-by-value. For example, it's not clear if
should yield the same behavior as if
were called. |
This reverts commit 42de3da.