wenjin272 commented on code in PR #1126: URL: https://github.com/apache/flink-agents/pull/1126#discussion_r4044342115
########## python/flink_agents/integrations/embedding_models/openai_embedding_model.py: ########## @@ -31,6 +35,166 @@ DEFAULT_REQUEST_TIMEOUT = 30.0 DEFAULT_BASE_URL = "https://api.openai.com/v1"; DEFAULT_MAX_RETRIES = 3 +DEFAULT_ENCODING_FORMAT = "float" +# Upper bounds shared with the OpenAI chat models and the Java connection. +MAX_OPENAI_TIMEOUT_SECONDS = 2_147_483.647 +MAX_OPENAI_RETRIES = 2_147_483_647 +MAX_DIMENSIONS = 2_147_483_647 # int range, as the Java connection requires +ENCODING_FORMATS = frozenset({"float", "base64"}) +# Typed request fields; additional_kwargs may not repeat them. +RESERVED_ADDITIONAL_KWARGS = frozenset( + {"model", "input", "encoding_format", "dimensions", "user"} +) + + +def _blank_to_none(value: Any, name: str) -> str | None: + """Require a string or None; blank strings are treated as absent, as in Java.""" + if value is None: + return None + if not isinstance(value, str): + msg = f"{name} must be a string, got: {type(value).__name__}" + raise TypeError(msg) + return value if value.strip() else None + + +def _check_additional_kwargs(additional_kwargs: Any) -> Dict[str, Any]: + """Require a mapping with non-blank string keys and no typed request field.""" + if not isinstance(additional_kwargs, Mapping): + msg = ( + f"additional_kwargs must be a map, got: {type(additional_kwargs).__name__}" + ) + raise TypeError(msg) + # Per-call keys are stringified as Java's Map keys are (the setup field already + # requires string keys); null and blank keys are rejected in both languages. + if any(key is None or not str(key).strip() for key in additional_kwargs): + msg = "additional_kwargs contains an empty key." + raise ValueError(msg) + additional_kwargs = {str(key): value for key, value in additional_kwargs.items()} + collisions = sorted(RESERVED_ADDITIONAL_KWARGS & additional_kwargs.keys()) + if collisions: + msg = ( + f"additional_kwargs must not contain the typed request fields {collisions}; " + "set them through the corresponding setup argument instead." + ) + raise ValueError(msg) + return additional_kwargs + + +def _check_encoding_format(encoding_format: Any) -> str: + """Validate encoding_format; a blank value means the default, as in Java.""" + if _blank_to_none(encoding_format, "encoding_format") is None: + return DEFAULT_ENCODING_FORMAT + if encoding_format not in ENCODING_FORMATS: + msg = ( + f"encoding_format must be one of {sorted(ENCODING_FORMATS)}, " + f"got: {encoding_format!r}" + ) + raise ValueError(msg) + return encoding_format + + +def _check_model(model: Any) -> str: + if not isinstance(model, str) or not model.strip(): + msg = "OpenAI embedding requires a non-empty 'model' (setup or per-call parameter)." + raise ValueError(msg) + return model + + +def _check_dimensions(dimensions: Any) -> int | None: + """Accept a positive integer (an integral float counts, as in Java).""" + if dimensions is None: + return None + if isinstance(dimensions, float) and dimensions.is_integer(): + dimensions = int(dimensions) + if ( + isinstance(dimensions, bool) + or not isinstance(dimensions, int) + or not 0 < dimensions <= MAX_DIMENSIONS + ): + msg = f"dimensions must be a positive integer, got: {dimensions!r}" + raise ValueError(msg) + return dimensions + + +def _usage_count(value: Any) -> int | None: + """A token count, or None when a compatible server sends something unusable.""" + if isinstance(value, str) and value.strip().isdecimal(): + return int(value) + if isinstance(value, bool): + return None + if isinstance(value, int): + return value + if isinstance(value, float) and value.is_integer(): + return int(value) + return None + + +def _decode_embedding(value: Any, *, base64_requested: bool) -> list[float]: + # With encoding_format="base64" the SDK hands the base64 string through untouched. + if isinstance(value, str): + if not base64_requested: + msg = "a string vector was returned although encoding_format was float" + raise TypeError(msg) + # validate=True so corrupted input raises instead of being silently truncated; + # unpadded input is accepted as Java's decoder accepts it. + padded = value + "=" * (-len(value) % 4) Review Comment: Could we only restore padding for completely unpadded values? With the current logic, malformed partial padding such as `AACAPw=` is normalized to `AACAPw==` and accepted, while Java's `Base64.getDecoder()` rejects it. This breaks the intended Java/Python parity and weakens the corrupted-response validation. Please keep partial padding invalid and add a regression test for it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
