-
Notifications
You must be signed in to change notification settings - Fork 881
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
Make ttl
a float
#3166
Make ttl
a float
#3166
Conversation
src/py/flwr/common/message.py
Outdated
Time-to-live for this message. | ||
""" | ||
# Create reply with error | ||
message = Message(metadata=self._create_reply_metadata(ttl), error=error) | ||
return message | ||
|
||
def create_reply(self, content: RecordSet, ttl: str) -> Message: | ||
def create_reply(self, content: RecordSet, ttl: float = DEFAULT_TTL) -> Message: |
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.
Shouldn't the default be the TTL of the instruction message?
We'd need to make ttl
an optional argument here and also add a check below that sets ttl
to the TTL of the instruction message if the user does not set the ttl
argument. We should probably also check that a user-provided TTL does not exceed the instruction message TTL.
def create_reply(self, content: RecordSet, ttl: float = DEFAULT_TTL) -> Message: | |
def create_reply(self, content: RecordSet, ttl: Optional[float] = None) -> Message: |
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.
my understanding was that theSuperNode
-side TTL
was to specify the max duration the TaskRes
is allowed to remain at the SuperLink
before it gets pulled by the ServerApp
(so they are different TTLs)
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 made the change to make it optional. But to keep things consistent, in this PR if the ttl
is unset, it will set DEFAULT_TTL
. In a follow up PR we'll replace it to what we agreed upon (the remaining time based on message creation and it's ttl from serverapp)
Co-authored-by: Daniel J. Beutel <daniel@flower.ai>
ttl
afloat
ttl
incommon.message
asDEFAULT_TTL=3600
ttl=""
withttl=DEFAULT_TTL
taskins/taskres
andmessage