Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Goodfire API Provider Support #1161
base: main
Are you sure you want to change the base?
Add Goodfire API Provider Support #1161
Changes from 19 commits
ee9a647
31f7d08
8397a93
17bb100
e2eb13a
63b1ad1
8c0a4d8
b466dea
a7cc7ad
7503d70
89a9ec0
1f9a9ca
65da3b1
254e0a8
15eda0a
65f507f
2af6a9a
89da81b
b628884
dfa4d0e
45d5609
06d8972
0f15aea
68ddb3b
0796eac
67cd044
0cd262f
3aa4d0a
6d6adf0
7597cc3
77b1f16
831f8de
004b982
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to force the user to be explicit here from the get-go (as once you get more namespaces you'll want to clearly disambiguate). When we started we allowed just a plain
gpt-4
orclaude-3
but as more providers came on line there were conflicts so we went back to requiring the fully namespaced name. You know this stack better than I though so take this as a suggestion only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response to Comment 2 (re: Model Name Namespacing)
In
src/inspect_ai/model/_providers/goodfire.py
:The change enforces fully namespaced model names by:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you return this instead:
Then you can include
ModelCall
information, which will be used in the viewer to show the underlying payload of the request to the Goodfire API (indispensable for debugging!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You noted this in your PR comments, but this absolutely has to be converted to async (as the sync version will hold up everything else in the process). If the goodfire client doesn't have an aysnc version then you should be able to just call
asynio.to_thread
and await that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goodfire has no async, and I have not been able to get this method to work at the moment. Will try again once more over the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can live without async but it will slow things down immeasurably and also make the task UI unresponsive. Definitely worth some time to get this to work properly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be logged elsewhere so we can remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove properties that just return the default (the tools ones + max_connections)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Response to Comment 10 (re: Default Properties)
collapse_user_messages()
method that just returnedTrue
collapse_assistant_messages()
method that just returnedTrue
max_tokens()
method (lines 203-211):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with other providers, please use constants here e.g.
raise pip_dependency_error(FEATURE, [PACKAGE])