fix: make otel util genai compatible with 3.9 #26
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
| categories: Sequence[str], | ||
| method_name: str, | ||
| *, | ||
| obj: Any | None = None, | ||
| error: Error | None = None, | ||
| obj: Union[Any, None] = None, | ||
| error: Union[Error, None] = None, | ||
| results: Sequence[EvaluationResult] | None = None, |
There was a problem hiding this comment.
The CompositeEmitter changes switch some annotations to typing.Union, but _dispatch still declares results: Sequence[EvaluationResult] | None. Python 3.9 cannot parse the | type-union syntax, so importing this module on 3.9 will still raise a SyntaxError. For the stated goal of 3.9 support, the remaining | usages in this file (e.g. Sequence[str] | None in the constructor and iterator helpers) also need to be converted to Union[...] or Optional[...].
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
There was a problem hiding this comment.
@codex How were you integrated on this and what can be done to get more helpful reviews from you?
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
| self, | ||
| metrics: Iterable[str] | None = None, | ||
| *, | ||
| invocation_type: str | None = None, | ||
| invocation_type: Union[str, None] = None, | ||
| options: Mapping[str, str] | None = None, |
There was a problem hiding this comment.
Although Union is now imported, the Evaluator initializer keeps metrics: Iterable[str] | None and options: Mapping[str, str] | None. These annotations still rely on the Python 3.10 union operator and will fail to parse on Python 3.9, so the module remains incompatible with the version this change targets. Converting these to Union[Iterable[str], None] (or Optional) is required for true 3.9 support.
Useful? React with 👍 / 👎.
| target: EvaluatorFactory, | ||
| *, | ||
| metrics: Sequence[str] | None = None, | ||
| invocation_type: str | None = None, | ||
| invocation_type: Union[str, None] = None, | ||
| options: Mapping[str, str] | None = None, |
There was a problem hiding this comment.
The registry’s _call_with_optional_params function continues to annotate parameters with metrics: Sequence[str] | None and options: Mapping[str, str] | None. These annotations use the | union operator that is unavailable in Python 3.9, so importing this module will still raise a syntax error even after this patch. To make the genai util package 3.9 compatible, these remaining unions need to be rewritten using typing.Union/Optional as done elsewhere.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
adityamehra
changed the title
zhirafovod
force-pushed
the
genai-utils-e2e-dev
branch
from
a8d3a8c to
90e5276
Compare
adityamehra
force-pushed
the
fix/make-genai-util-compatible-with-python3.9
branch
2 times, most recently
from
0da85b4 to
022f242
Compare
Use Union
adityamehra
force-pushed
the
fix/make-genai-util-compatible-with-python3.9
branch
from
022f242 to
35edb49
Compare
adityamehra
changed the title
Description
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.