feat: code_interpreter#232
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
nrfulton
force-pushed
the
nathan/tools_reqlib
branch
from
b568898 to
b72ac1b
Compare
Tracking this ergonomic issue in generative-computing#234.
We should still think about this.
| def to_validationresult_reason(self): | ||
| """Maps an ExecutionResult to a ValidationResult reason. | ||
|
|
||
| TODO: Downstream use of this method is really hacky. A far better solution is for `ExecutionResult` to implement the `ValidationResult` interface. |
| """Execute code and return result.""" | ||
|
|
||
|
|
||
| class StaticAnalysisEnvironment(ExecutionEnvironment): |
There was a problem hiding this comment.
Also as part of #234's work on subtyping ValidationResult, we could have a StaticAnalysisReseult instead of ExecutionResult.
| skip_message=f"Unauthorized imports detected: {', '.join(unauthorized)}", | ||
| ) | ||
|
|
||
| try: |
There was a problem hiding this comment.
This requirement has already been added to the pyproject.toml. It should probably be moved to the top-level.
I am going to merge this PR without changing this because we already had this non-top-level import in the original version of this code (in reqlib).
@0xCUB3 can you comment on why we would be doing this here and catching the import error?
There was a problem hiding this comment.
I honestly didn't consider that when writing this code. If the dependency is already in pyproject.toml as a req'd package, the import should just be at the top level since the package will always be installed. The only reason to keep this try/except pattern would be if we want to make this an optional dependency (moved to an extras group in pyproject.toml for instance), else it's redundant error handling.
2 participants
This PR focuses on factoring @0xCUB3's code out of the
reqliband into a newtoolsmodule. I also added sometool/code_interpreter-specific requirements.The PR message here used to mention a bunch of other tool use stuff, which I've moved into #235 so that progress on that work doesn't block merging this PR.