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

[issue_4156] Improves handling of empty or blank input for openai procedures #2

Open
wants to merge 4 commits into
base: 5.25
Choose a base branch
from

Conversation

mpetrini-larus
Copy link
Owner

@mpetrini-larus mpetrini-larus commented Nov 14, 2024

Fixes neo4j-contrib#4156

Improves handling of empty or blank input for OpenAI procedures

Proposed Changes (Mandatory)

Here is a brief list of proposed changes to fix the issue:

  • Added input validation to prevent unnecessary API calls toOpenAI when the input is empty or null.
  • Introduced a boolean flag to allow configuration of the behaviour:
    • If set to True, an exception is raised when an empty (or blank) input is encountered.
    • If False, the API call is silently skipped.
  • This change improves efficiency by streamlining the process and giving flexibility in handling empty input.

Copy link

@vga91 vga91 left a comment

Choose a reason for hiding this comment

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

Direi Eccellente Smithers, giusto un po' di cambiamentini e test in più.

Poi inserisci nel docs/asciidoc/modules/ROOT/pages/ml/openai.adoc,
circa alla riga 32 questa riga:

| failOnError | If true (default), the procedure fails in case of empty, blank or null input 

extended/src/main/java/apoc/ml/OpenAI.java Outdated Show resolved Hide resolved
extended/src/main/java/apoc/ml/OpenAI.java Outdated Show resolved Hide resolved
TestUtil.testCallEmpty(db, "CALL apoc.ml.openai.completion(null, $apiKey, $conf)",
Map.of("apiKey", openaiKey, "conf", Map.of(FAIL_ON_ERROR_CONF, false))
);
}
}
Copy link

Choose a reason for hiding this comment

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

Aggiungi pure dei test con lista vuota tipo:

    @Test
    public void embeddingsWithEmptyFails() {
        assertEmptyInputFails(db, "CALL apoc.ml.openai.chat([], $apiKey, $conf)",
                Map.of("apiKey", openaiKey, "conf", emptyMap())
        );
    }
    
    @Test
    public void embeddingsWithEmptyReturnsEmptyIfFailOnErrorFalse() {
        TestUtil.testCallEmpty(db, "CALL apoc.ml.openai.embeddings([], $apiKey, $conf)",
                Map.of("apiKey", openaiKey, "conf", Map.of(FAIL_ON_ERROR_CONF, false))
        );
    }

e la classe MLTestUtil.java la cambi in questo modo, visto che mo abbiamo a disposizione questo stupendo ExtendedTestUtil.assertFails:

public class MLTestUtil {
    
    public static void assertNullInputFails(GraphDatabaseService db, String query, Map<String, Object> params) {
        ExtendedTestUtil.assertFails(db, query, params, ERROR_NULL_INPUT);
    }
    
    public static void assertEmptyInputFails(GraphDatabaseService db, String query, Map<String, Object> params) {
        ExtendedTestUtil.assertFails(db, query, params, ERROR_EMPTY_OR_BLANK_INPUT);
    }
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

dovrei aver sistemato come suggerito!
mi facci sapere se c'è altro, o mi sono dimenticato qualcosa ^^

@vga91
Copy link

vga91 commented Nov 15, 2024

:emoticon-libidine: lgtm questa puoi chiuderla, la PR sulla repo principale la devo fare io

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