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

Fixes 3611: ML API procedures handle null values being passed in better #439

Closed
wants to merge 5 commits into from

Conversation

vga91
Copy link
Owner

@vga91 vga91 commented Mar 6, 2024

Fixes https://github.com/neo4j-contrib/neo4j-apoc-procedures/issues/3611

  • vertexai and openai embeddings return null rows with null list values

  • embeddings, chat completion, completion and image procedures return null if the input parameter is null

  • not implemented null check in apoc.ml..custom procedures, since we can potentially pass a null parameter (e.g. to call this RestAPI)

  • TODO: open Trello Card for the following note?

Additional notes

Would be great to handle errors like these, but we cannot implement it since is part of APOC Core:

CALL apoc.ml.openai.chat([ {role:"user", content:null}], $apiKey, $conf)

We should do something like this:

        @Override
        public InputStream getInputStream() throws IOException {
            if (con instanceof HttpURLConnection httpConn && httpConn.getResponseCode() >= 400) 
            {
                // return ErrorStream as a RuntimeException 
                String errMsg = new String(httpConn.getErrorStream().readAllBytes());
                throw new RuntimeException("Error during HTTP call:\n" + errMsg);
            }
            
            return toLimitedIStream(con.getInputStream(), getLength());
        }

@vga91 vga91 force-pushed the issue-3611 branch 2 times, most recently from 6f1ee94 to 549038f Compare March 6, 2024 14:41
@vga91 vga91 marked this pull request as draft March 6, 2024 22:25
@vga91 vga91 marked this pull request as ready for review March 19, 2024 00:01
Map<Boolean, List<String>> collect = texts.stream()
.collect(Collectors.groupingBy(Objects::nonNull));

List<String> nonNullTexts = collect.get(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

qui non servirebbe vedere se la collezione è nulla di modo da poter lanciare un eccezione prima di fare la richiesta?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sì in effetti è meglio, ho aggiunto il check con l'eccezione sopra.
Questa parte l'ho lasciata cosi perché è richiesto dalla issue:

But we should probably just filter those out in the embeddings call and don't forward them and return the result row with a null value for embedding.

@@ -132,13 +147,19 @@ public Stream<MapResult> completion(@Name("prompt") String prompt, @Name("api_ke
"usage": { "prompt_tokens": 5, "completion_tokens": 7, "total_tokens": 12 }
}
*/
if (prompt == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

qui ugualmente non sarebbe meglio fallire prima di fare la richiesta?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Cambiato

@RobertoSannino
Copy link
Collaborator

LGTM

@vga91 vga91 closed this Apr 15, 2024
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