Otto Assistant

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
matthiaaas
User
Beiträge: 4
Registriert: Sonntag 16. Juni 2019, 10:40

👋

Ursprünglich für meinen Raspberry Pi habe ich mir einen Sprachassistenten programmiert. "Otto" funktioniert soweit eigentlich gut, dennoch wollte ich einmal das Forum darum bitten, einen Blick auf das Github Repo zu werfen und die ganzen Programmierfehler auszumerzen.

https://github.com/matthiaaas/otto-assistant

Kritik oder Anmerkungen gerne hier kommentieren oder sogar eine Pull Request erstellen, sofern man denn die Zeit findet.

Vielen Dank!
__deets__
User
Beiträge: 14529
Registriert: Mittwoch 14. Oktober 2015, 14:29

Eine Sache die gleich auffällig ist: die Notwendigkeit Pakete je nach Plattform auszukommentieren ist Mist. Das geht besser: https://pip.pypa.io/en/stable/reference ... specifiers


Und mir ist es ein bisschen zu implizit angesprochen, dass die eigentliche sprachverarbeitung online erfolgt. Das es nicht anders geht - ok. Aber es wird nur in der trouble shooting Sektion erwähnt. Dabei ist es eine der Zentralen sorgen der Benutzer.
matthiaaas
User
Beiträge: 4
Registriert: Sonntag 16. Juni 2019, 10:40

__deets__ hat geschrieben: ↑Sonntag 16. Juni 2019, 11:47 Eine Sache die gleich auffällig ist: die Notwendigkeit Pakete je nach Plattform auszukommentieren ist Mist. Das geht besser: https://pip.pypa.io/en/stable/reference ... specifiers


Und mir ist es ein bisschen zu implizit angesprochen, dass die eigentliche sprachverarbeitung online erfolgt. Das es nicht anders geht - ok. Aber es wird nur in der trouble shooting Sektion erwähnt. Dabei ist es eine der Zentralen sorgen der Benutzer.
Okay. Werde ich ändern. Danke!

Wie schätzt du den Code ein / Was fällt dir dort auf?
Sirius3
User
Beiträge: 17741
Registriert: Sonntag 21. Oktober 2012, 17:20

`core` ist ein schlechter Name für ein Paket, weil zu allgemein, ebenso `run.py` als Scriptname.
Innerhalb der Module daher am besten auch relative Importe verwenden, dann ist das umbenennen des Pakets kein Problem.

In assistant.py: das globale ›instance‹ gehört weg. Dokstrings gehören in die Klasse, bzw. Methode.
Das ›run‹ gehört nicht in Assistant.__init__, denn __init__ sollte ein Objekt initialisieren und dann zurückkehren und nicht ewig laufen. Ebenso sollte das was in setup steht direkt in __init__ sein, und greet nicht darin aufgerufen werden.
In ›run‹, Du hast eigentlich eine while-not-stop-Schleife, weil Du ja als erstes gleich auf stop prüfst.
Wenn in jedem except ein `break` steht, dann sollte das try einfach außerhalb der while-Schleife stehen. Da das allgemeine except nichts macht, außer den Traceback darstellen, kann es ganz weg.
Wenn Du eine andere Ausgabe dort willst, gehört das auch nicht in ›run‹ sondern in ›main‹.
Alles innerhalb des if mit `listen_for_keyword` gehört in eine Funktion, weil ›run‹ sonst zu lange ist und zu weit eingerückt ist. `continue` vermeiden.

modules.log kann weg, Du benutzt doch schon logging, und das Zusatzmodul kann nichts, was logging nicht schon machen würde.
In `matching` ist in test_match und check_match viel doppelter Code. Dass die json-Datei ständig neu geladen wird, ist umständlich. Du hast überall viele Klassen, warum hier, wo es wirklich Sinn macht eine Matchingklasse zu haben, nicht??
In `get_match` iterierst Du über einen Index, wo soll denn der TypeError auftreten? Oje, jetzt seh ich das erst. Du benutzt die Tatsache, dass Du in der Debug-Meldung auf cmd zugreifst und dass das None sein kann bei einem Indexzugriff dann einen TypeError auslöst. Das ist die Umständlichste Art, die ich je gesehen habe um `if cmd is not None:` auszudrücken.

In › execute_match‹ nicht direkt auf __Methoden__ wie __contains__ zugreifen, benutze ›in‹.

In ›stt.py‹ wird ›global‹ benutzt, so dass ich hier nicht mehr durchblicke. So lange das nicht aufgeräumt ist, was wohl bedeutet, es komplett umzuschreiben, lohnt sich kein Blick hinein.
matthiaaas
User
Beiträge: 4
Registriert: Sonntag 16. Juni 2019, 10:40

Sirius3 hat geschrieben: ↑Sonntag 16. Juni 2019, 12:18 `core` ist ein schlechter Name für ein Paket, weil zu allgemein, ebenso `run.py` als Scriptname.
Innerhalb der Module daher am besten auch relative Importe verwenden, dann ist das umbenennen des Pakets kein Problem.

In assistant.py: das globale ›instance‹ gehört weg. Dokstrings gehören in die Klasse, bzw. Methode.
Das ›run‹ gehört nicht in Assistant.__init__, denn __init__ sollte ein Objekt initialisieren und dann zurückkehren und nicht ewig laufen. Ebenso sollte das was in setup steht direkt in __init__ sein, und greet nicht darin aufgerufen werden.
In ›run‹, Du hast eigentlich eine while-not-stop-Schleife, weil Du ja als erstes gleich auf stop prüfst.
Wenn in jedem except ein `break` steht, dann sollte das try einfach außerhalb der while-Schleife stehen. Da das allgemeine except nichts macht, außer den Traceback darstellen, kann es ganz weg.
Wenn Du eine andere Ausgabe dort willst, gehört das auch nicht in ›run‹ sondern in ›main‹.
Alles innerhalb des if mit `listen_for_keyword` gehört in eine Funktion, weil ›run‹ sonst zu lange ist und zu weit eingerückt ist. `continue` vermeiden.

modules.log kann weg, Du benutzt doch schon logging, und das Zusatzmodul kann nichts, was logging nicht schon machen würde.
In `matching` ist in test_match und check_match viel doppelter Code. Dass die json-Datei ständig neu geladen wird, ist umständlich. Du hast überall viele Klassen, warum hier, wo es wirklich Sinn macht eine Matchingklasse zu haben, nicht??
In `get_match` iterierst Du über einen Index, wo soll denn der TypeError auftreten? Oje, jetzt seh ich das erst. Du benutzt die Tatsache, dass Du in der Debug-Meldung auf cmd zugreifst und dass das None sein kann bei einem Indexzugriff dann einen TypeError auslöst. Das ist die Umständlichste Art, die ich je gesehen habe um `if cmd is not None:` auszudrücken.

In › execute_match‹ nicht direkt auf __Methoden__ wie __contains__ zugreifen, benutze ›in‹.

In ›stt.py‹ wird ›global‹ benutzt, so dass ich hier nicht mehr durchblicke. So lange das nicht aufgeräumt ist, was wohl bedeutet, es komplett umzuschreiben, lohnt sich kein Blick hinein.
Okay. Ich werde dann mal alles nacheinander ändern. Danke!

Aber ist in run() eine Funktion doch eigentlich gar nicht notwendig? Weil das Ganze brauche ich ja nur an einer Stelle und zwar in run(). Außerdem würde mir das ja nur eine Einrückung ersparen. Das kann ich aber auch erreichen, indem ich das

Code: Alles auswählen

if stt.listen_for_keyword():
mit einem 'continue' versehe. Dann kann alles andere ja einmal wieder zurück eingerückt werden.

In dem Zusammenhang: Ich verstehe noch nicht ganz, warum man 'continue' vermeiden sollte.
Sirius3
User
Beiträge: 17741
Registriert: Sonntag 21. Oktober 2012, 17:20

Du hast den try-Block, den while-Block und den if-Block, für mich sind das 3 Ebenen.
›continue‹ ist nicht gut, weil es den Programmfluß mittendrin unterbricht.

Wenn ich mir die Klasse ›Assistant‹ nochmal anschaue, dann ist das eigentlich gar keine Klasse, weil es nur aus ›run‹ besteht.

Code: Alles auswählen

import logging
from .modules import stt, tts, matching, tasks
logger = logging.getLogger(__name__)

def greet():
    print("")
    print(r"         __       __              ")
    print(r"        /\ \__   /\ \__           ")
    print(r"  ___   \ \ ,_\  \ \ ,_\    ___   ")
    print(r" / __`\  \ \ \/   \ \ \/   / __`\ ")
    print(r"/\ \ \ \  \ \ \_   \ \ \_ /\ \ \ \ ")
    print(r"\ \____/   \ \__\   \ \__\\ \____/")
    print(r" \/___/     \/__/    \/__/ \/___/ ")
    print("")
    print("Basic usage:\n")
    print(" Otto *sound*, wie spät ist es?\n")
    print(" Otto *sound*, wie ist das Wetter?")
    print("")
    tts.say("Hallo, wie kann ich behilflich sein?")

def listen():
    logger.debug("Listen for command...")
    # listen for text input
    audio = stt.listen()
    # try resolving input
    input = stt.recognize(audio)
    # check if text input received
    if not input:
        logger.info("Couldn't resolve audio...")
    else:
        logger.info("Catched input '{}'...".format(input))
        cmd = matching.get_match(input)
        matching.execute_match(cmd)

def run():
    try:
        while True:
            # listen for keyword
            # wake up on recognized keyword
            if stt.listen_for_keyword():
                listen()
    except KeyboardInterrupt:
        # user interrupted program
        logger.info("Detected keyboard interruption...")

def main():
    stt.setup()
    tts.setup()
    greet()
    run()
Ich vermute, `stt.setup` und `tts.setup` sollten nach der Resturkturierung tatsächlich Klassen werden, und die Instanzen dann an `run` übergeben werden, weil Du in den Modulen mit ›global‹ herumeierst.
matthiaaas
User
Beiträge: 4
Registriert: Sonntag 16. Juni 2019, 10:40

Sirius3 hat geschrieben: ↑Sonntag 16. Juni 2019, 13:32 Du hast den try-Block, den while-Block und den if-Block, für mich sind das 3 Ebenen.
›continue‹ ist nicht gut, weil es den Programmfluß mittendrin unterbricht.

Wenn ich mir die Klasse ›Assistant‹ nochmal anschaue, dann ist das eigentlich gar keine Klasse, weil es nur aus ›run‹ besteht.

Code: Alles auswählen

import logging
from .modules import stt, tts, matching, tasks
logger = logging.getLogger(__name__)

def greet():
    print("")
    print(r"         __       __              ")
    print(r"        /\ \__   /\ \__           ")
    print(r"  ___   \ \ ,_\  \ \ ,_\    ___   ")
    print(r" / __`\  \ \ \/   \ \ \/   / __`\ ")
    print(r"/\ \ \ \  \ \ \_   \ \ \_ /\ \ \ \ ")
    print(r"\ \____/   \ \__\   \ \__\\ \____/")
    print(r" \/___/     \/__/    \/__/ \/___/ ")
    print("")
    print("Basic usage:\n")
    print(" Otto *sound*, wie spät ist es?\n")
    print(" Otto *sound*, wie ist das Wetter?")
    print("")
    tts.say("Hallo, wie kann ich behilflich sein?")

def listen():
    logger.debug("Listen for command...")
    # listen for text input
    audio = stt.listen()
    # try resolving input
    input = stt.recognize(audio)
    # check if text input received
    if not input:
        logger.info("Couldn't resolve audio...")
    else:
        logger.info("Catched input '{}'...".format(input))
        cmd = matching.get_match(input)
        matching.execute_match(cmd)

def run():
    try:
        while True:
            # listen for keyword
            # wake up on recognized keyword
            if stt.listen_for_keyword():
                listen()
    except KeyboardInterrupt:
        # user interrupted program
        logger.info("Detected keyboard interruption...")

def main():
    stt.setup()
    tts.setup()
    greet()
    run()
Ich vermute, `stt.setup` und `tts.setup` sollten nach der Resturkturierung tatsächlich Klassen werden, und die Instanzen dann an `run` übergeben werden, weil Du in den Modulen mit ›global‹ herumeierst.
Der Relative Import funktioniert nicht. siehe hier.
Ist aber glaube ich auch nicht wirklich groß von Bedeutung und notwendig...
Antworten