Default state for map actions + other minor testing-related fixes #466
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8610804 in 53 seconds
More details
- Looked at
154lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_xafvt3YBnj9M4xHL
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
A preview of 7e49f99 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/466 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 607d9f3 in 16 seconds
More details
- Looked at
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_1bOQSmVij8ATCIv1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 10705c4 in 26 seconds
More details
- Looked at
41lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. burr/tracking/server/schema.py:96
- Draft comment:
Ensure thatmodel_validateis the correct method to use here instead ofparse_obj. This change is applied consistently across multiple lines (96, 99, 102, 108, 112, 119), so verify that this method is available and behaves as expected in the context ofpydanticmodels. - Reason this comment was not posted:
Confidence changes required:50%
The change fromparse_objtomodel_validateis consistent across multiple lines. This change should be verified for correctness and consistency with the rest of the codebase.
Workflow ID: wflow_TGUKCWu99gY85EsZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
elijahbenizzy
force-pushed
the
default_state_map_actions
branch
from
10705c4 to
4a90c0b
Compare
THis just defaults to the passed-in state. This is an easy way to get it to have a reasonable default and think about it less -- a common use-case as well, when multiple actions want to function on the same state.
This produced errors, doing some cleanup
elijahbenizzy
force-pushed
the
default_state_map_actions
branch
from
4a90c0b to
e5b2e82
Compare
Deprecation warnings -- this is the new/better way to do it. Also makes pydantic>1 for the model_validate methods
elijahbenizzy
force-pushed
the
default_state_map_actions
branch
from
e5b2e82 to
7e49f99
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7e49f99 in 80 minutes and 28 seconds
More details
- Looked at
55lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pyproject.toml:82
- Draft comment:
Good update to require Pydantic v2 - this matches the code changes to usemodel_validateinstead of the deprecatedparse_obj. - Reason this comment was not posted:
Confidence changes required:0%
The PR changes parse_obj to model_validate in schema.py. This is a good change since parse_obj is deprecated in Pydantic v2. The PR also updates the pydantic dependency to >1 in pyproject.toml to ensure compatibility. This is a good change that ensures proper Pydantic v2 usage.
Workflow ID: wflow_3LkqA6Cg1h2dVDAM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Changes
Default state for
MapActionsis now the state that was provided. Lowers the amount needed for a common use-case.Also did a bit of improvements on the testing side as well -- removed warnings from test suite!
Important
Set default state for
MapActionsand improve test clarity by renaming classes and adding tests.MapActionsnow uses the provided state, reducing setup for common use-cases inparallelism.py.test_map_actions_default_state()intest_parallelism.pyto verify default state behavior.test_application.pyfor clarity:TestActionWithoutContexttoActionWithoutContext,TestActionWithContexttoActionWithContext,TestActionWithKwargstoActionWithKwargs, andTestActionWithContextTracertoActionWithContextTracer.parse_objwithmodel_validateinschema.pyforBeginEntryModel,EndEntryModel,BeginSpanModel,EndSpanModel,AttributeModel, and streaming events.pydanticversion constraint inpyproject.tomlto>1.This description was created by
for 7e49f99. It will automatically update as commits are pushed.