Skip to content

Lazily evaluate conditions #201

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

Merged
merged 5 commits into from
Nov 9, 2022
Merged

Conversation

CamDavidsonPilon
Copy link
Contributor

@CamDavidsonPilon CamDavidsonPilon commented Dec 6, 2021

I am writing some unit tests for my package that uses this library, and I take advantage of the BLINKA_FORCECHIP and BLINKA_FORCEBOARD environment variables to "mock" a board. However, my recent builds start failing with an error thrown in this library. Try to debug what might be going wrong, I came across a problem in the following LOC:

https://github.com/adafruit/Adafruit_Python_PlatformDetect/blob/main/adafruit_platformdetect/board.py#L619-L624

What was happening was that while any_raspberry_pi_40_pin would evaluate to True, any_raspberry_pi would thow an error (internally, it's looking for a specific file, which doesn't exist in my testing environment). I was actually surprised, since I know that any is lazy: it will prematurely exist once it finds the first True - however, the way the code is currently written, Python first creates the list [any_raspberry_pi_40_pin, any_raspberry_pi, ..., any_maaxboard], and then calls any on it. This means:

  1. an error thrown in any of the any_ properties will be raised immediately (what I was observing),
  2. we evaluate every condition, even if those conditions aren't checked. This has a performance cost. Most any_ are just lookups, but some, like any_raspberry_pi, involve an IO operation. Some quick tests suggest ~10% improvement in time taken.

Here's a simplified version of 1)

class Klass():
    @property
    def any_embedded_linux(self):
        return any([
            self.okay_prop,
            self.other_prop,
            self.bad_prop,
        ])

    @property
    def okay_prop(self):
        return True

    @property 
    def bad_prop(self):
        raise ValueError

    @property 
    def other_prop(self):
        return False

k = Klass()
print(k.any_embedded_linux)

and 2)

class Klass():

    @property
    def any_embedded_linux(self):
        return any([
            self.okay_prop,
            self.other_prop,
            self.sleepy_prop,
        ])

    @property
    def okay_prop(self):
        return True

    @property 
    def other_prop(self):
        return False

    @property
    def sleepy_prop(self):
        import time
        time.sleep(10)

k = Klass()
print(k.any_embedded_linux)

Anyways, a solution is to turn the list into a generator. This solves 1) and 2), but has a bit of a code smell to it (it's not obvious why we are doing this at first look, only after this explanation is provided).

Let me know your thoughts!

self.any_maaxboard,
]
)
def _generate_linux_conditions():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an alternative way to do this is something like:

for prop in ["any_raspberry_pi_40_pin", "any_raspberry_pi", ...]:
      yield getattr(self, prop)

@CamDavidsonPilon CamDavidsonPilon changed the title First note that in the old code, the entire list is evaluated before … Lazily evaluate conditions Dec 6, 2021
…the any() is called. Changing this code solves two issues. 1) if there is an error in one of these conditions, then the entire function won't error out immediately but may hit a True condition and exit early. 2) Most of these conditions are inexpensive lookups, but some, like 'any_raspberry_pi' perform lots of logic including an IO lookup
@makermelissa
Copy link
Collaborator

Sorry for the delay. I just merged new updates since you had submitted this and fixed any errors as a result of that since that was my fault.

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this. I hope it helps performance.

@makermelissa makermelissa merged commit f6ca7ad into adafruit:main Nov 9, 2022
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.

2 participants