Skip to content
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: Add compute-feature to BaseBone #639

Merged
merged 30 commits into from
Jul 10, 2023
Merged

feat: Add compute-feature to BaseBone #639

merged 30 commits into from
Jul 10, 2023

Conversation

ArneGudermann
Copy link
Contributor

@ArneGudermann ArneGudermann commented Feb 1, 2023

Extension for #623. With this PR you can now pass a compute parameter to each bone.
The compute parameter is a dataclass and consists of the following attributes.

@dataclass
class Compute:
    fn: callable  # The callable computing the value
    threshold: ThresholdValue   # The value caching interval
    raw: bool = True  # defines whether the value is used as is, or is passed to bone.fromClient

The ThresholdValue is a dataclass to and defiend like this.

class ThresholdMethods(Enum):
    Always = 0
    Until = 1
    Once = 2


@dataclass
class ThresholdValue:
    method: ThresholdMethods = ThresholdMethods.Always
    seconds: int = 0

@phorward phorward changed the title Feature/computed computed-parameter for all bones Feb 1, 2023
@phorward phorward added viur-meeting Issues to discuss in the next ViUR meeting feature New feature or request labels Feb 1, 2023
@phorward
Copy link
Member

phorward commented Feb 1, 2023

Hello @ArneGudermann,

thanks for starting this pull request draft.

I think you've used my first proposal from my comment in #629 which you got by email, but I did several revisions on it. I think, that Threshold is the wrong value for this, and named it (in a later version of the comment) into ComputeInterval. Can you please re-check this and merge this proposal into this pull request?

For clarification, here's my comment from the link above as a copy. I've tested it and it should entirely fulfill all requirements on issue #692.


Hello @ArneGudermann,

I think that this namedtuple + enum definition would generally be the better choice in BaseBone. This draft defines all required features, and might also answer your question: The compute-definition might be configured to allow for raw values or to pass them through the bone's fromClient.

from dataclasses import dataclass
from enum import Enum, unique

@unique
class ComputeInterval(Enum):
    ALWAYS = 1
    ONCE = 2
    SECONDS = 3

    @classmethod
    def seconds(cls, value):
        instance = cls.SECONDS
        instance._value_ = value
        return instance

@dataclass
class Compute:
    fn: callable  # the callable computing the value
    interval: ComputeInterval = ComputeInterval.ALWAYS   # the value caching interval
    raw: bool = True  # defines whether the value is used as is, or is passed to bone.fromClient

Afterwards, compute_me can be defined this way:

def testme():
    return "123.1"

compute_me = NumericBone(
    compute=Compute(testme)
)

# Compute(fn=<function testme at 0x7f790476cee0>, interval=<ComputeInterval.ALWAYS: 1>, raw=True)

compute_me_securely = NumericBone(
    compute=Compute(testme, raw=False)
)

# Compute(fn=<function testme at 0x7f790476cee0>, interval=<ComputeInterval.ALWAYS: 1>, raw=False)

compute_me_2min = NumericBone(
    compute=Compute(testme, ComputeInterval.seconds(60 * 2))
)

# Compute(fn=<function testme at 0x7f790476cee0>, interval=<ComputeInterval.SECONDS: 120>, raw=True)

The readOnly is automatically enforced by the constructor.

What do you think? I want to avoid enforcing to only accept str and parse them always via fromClient, as this might result in values generated, converted to a parse-able representation to become the same representation as originally created in the end, which is an unnecessary round trip.

@ArneGudermann
Copy link
Contributor Author

Hello @phorward
Thanks for your answer. I have one thing.

compute_me_2min = NumericBone(
    compute=Compute(testme, ComputeInterval.seconds(60 * 2))
)

Unfortunately this does not work because when I clone the skeleton before passing it to the compute function I get 120 as the value of the enum.

@phorward
Copy link
Member

phorward commented Feb 2, 2023

Unfortunately this does not work because when I clone the skeleton before passing it to the compute function I get 120 as the value of the enum.

You're right and I apologize. I really miss Rust's enums in Python, as

enum ComputeInterval {
    Always,
    Once,
    Seconds(usize),
}

is what I wanted to imitate here.

Anyway, could we make it simply this way first?

from dataclasses import dataclass

@dataclass
class Compute:
    fn: callable  # the callable computing the value
    seconds: int = 0  # interval in seconds, default is always
    raw: bool = True  # defines whether the value is used as is, or is passed to bone.fromClient

So seconds is just an int, which defines 0 as always, anything < 0 as once and >0 as timeout?

compute_me = NumericBone(
    compute=Compute(testme)
)

#Compute(fn=<function testme at 0x7fe27e97fd90>, seconds=0, raw=True)

compute_me_securely = NumericBone(
    compute=Compute(testme, seconds=-1, raw=False)
)

#Compute(fn=<function testme at 0x7fe27e97fd90>, seconds=-1, raw=False)

compute_me_2min = NumericBone(
    compute=Compute(testme, seconds=60 * 2)
)

#Compute(fn=<function testme at 0x7fe27e97fd90>, seconds=120, raw=True)

core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Outdated Show resolved Hide resolved
@phorward phorward removed the viur-meeting Issues to discuss in the next ViUR meeting label Feb 3, 2023
@ArneGudermann ArneGudermann marked this pull request as ready for review March 1, 2023 15:49
core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Outdated Show resolved Hide resolved
@phorward
Copy link
Member

phorward commented Apr 4, 2023

@sveneberth @ArneGudermann I've made a reworked proposal based on Arne's draft. It works, but I think some things could be improved as well. Can you please make a review on this?

core/bones/base.py Outdated Show resolved Hide resolved
core/bones/base.py Show resolved Hide resolved
core/bones/base.py Show resolved Hide resolved
@phorward phorward requested review from phorward and sveneberth April 5, 2023 20:44
@phorward phorward added the viur-meeting Issues to discuss in the next ViUR meeting label Apr 13, 2023
core/bones/base.py Outdated Show resolved Hide resolved
Co-authored-by: Jan Max Meyer <[email protected]>
phorward
phorward previously approved these changes Apr 14, 2023
Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

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

Can we resolve https://github.com/viur-framework/viur-core/pull/639/files#r1166918242 before merging?

For the caching thing I've create an issue #709

@phorward phorward removed the viur-meeting Issues to discuss in the next ViUR meeting label Apr 14, 2023
@phorward
Copy link
Member

Can we resolve https://github.com/viur-framework/viur-core/pull/639/files#r1166918242 before merging?

This refers to #639, can you clarify?

For the caching thing I've create an issue #709

Thanks! 👍

Copy link
Contributor Author

@ArneGudermann ArneGudermann left a comment

Choose a reason for hiding this comment

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

I fine with it. I think we should merge it. It not break anything and we can test it and improve it.

@sveneberth
Copy link
Member

Can we resolve https://github.com/viur-framework/viur-core/pull/639/files#r1166918242 before merging?

This refers to #639, can you clarify?

For the caching thing I've create an issue #709

Thanks! +1

The link was stupid... This was the issue, that has been resolved: #639 (comment)

sveneberth
sveneberth previously approved these changes Apr 26, 2023
@phorward phorward self-requested a review April 28, 2023 09:22
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

OK let's do this.

@phorward phorward merged commit 49e453e into develop Jul 10, 2023
@phorward phorward deleted the feature/computed branch July 10, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants