-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat(experiments): ability to specify concurrency in run_experiment and evaluate_experiment #4189
feat(experiments): ability to specify concurrency in run_experiment and evaluate_experiment #4189
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
This MR might be misguided as I'm not really observing any concurrency in the current implementation 🤔 .. Seems like tasks are always evaluated in sequence? Curious to get your guidance |
Hi @anton164, this PR does correctly wire up configuring the concurrency of our experiment runners—if the tasks that are being run are async. We generate a sequence of tasks to run (concurrently if possible) then submit them to our executor. Would it be possible to rewrite your task as a coroutine function instead and report back if the default level of concurrency works for you? |
Thanks @anticorrelator -- concurrency works in my experiments with a coroutine task, but the level of concurrency is not sufficient. I’d like to use ~20 concurrency for my batch experiments. I’ve updated the PR to fix a typo and add docs about the task needing to be a coroutine |
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.
Thanks so much for the contribution @anton164!
To fully wire this up we need to add a one more thing:
On line 416 we should pass the concurrency parameter to evaluate_experiment
.
Please let me know if setting the concurrency alone gives you the desired throughput. There are more things you can adjust if you run into throughput issues while using LLM evaluators too.
Good catch @anticorrelator, updated! Thanks for the quick reviews |
Thanks for your contribution @anton164 ! 💯 |
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.
LGTM!
@anton164 Looks like there's a line length linting issue in the docstring before we can merge |
@anticorrelator argh, sorry about that. Hadn't set up linting in my dev environment. I set it up now and confirmed that linting for this file no longer fails after fixing the line ending. |
* main: (58 commits) Update OpenAPI spec (#4260) Default to adding span annotations to queue (#4259) docs: Add human feedback notebook tutorial (#4257) refactor: daemon task abstraction (#4255) feat(auth): create system key (#4235) chore: add vision fixture (#4250) docs: fix variable name typo in run experiments doc (#4249) build(deps): bump certifi (#4147) chore(main): release arize-phoenix 4.24.0 (#4225) chore(main): release arize-phoenix-evals 0.15.0 (#3920) feat(experiments): ability to specify concurrency in run_experiment and evaluate_experiment (#4189) ci(evals): limit `mistralai` below version 1.0 (#4241) feat(auth): user / system api key resolvers (#4232) feat(auth): add user role, exclude system in user lists (#4229) docs: Add multimodal image reasoning tutorial with llama index (#4210) chore(main): release arize-phoenix 4.23.0 (#4216) docs: No subject (GITBOOK-818) build(deps): bump streamlit (#4214) fix: Propagate span annotation metadata to examples on all mutations (#4195) feat(auth): users table in settings (#4221) ...
(#4186)