Support for PydanticV2 #212
Conversation
- Temporarily comment out RedBase session attribute as it was causing issues with re-writing the value - Converted update_forward_refs to model_rebuild in _setup.py - set type on mac_process_count in sessions.py - Implemented ClassVar when dealing with session variable to fix inheritence - Convert Config Meta Class into model_config attribute - set default values Task class as in pydanticv2 they were set to required=True as default - _delayed_kwargs now ClassVar to be passed correctly
- run bump-pydantic script to make changes where applicable (Didn't seem to make that many changes) - fix session variable overwrite in test_core.py - add variable to root_validator (to get tests to run) but this needs migrating as root_validator is depreceated - test results: 43 failed, 1739 passed, 23 skipped, 8 xfailed, 3 xpassed, 17654 warnings
- always=True is depreceated, alternate solution to set model config validate_defualt to True to force checking on default values (which were set as part of migration) - Had to apply some additional typing to allow for that setting (Mainly setting some Union None as well as callable values) - values argument passed into validators now has nested data attribute in class. Had to append .data to any call of values - fixed manual validator changes that the bump script couldn't do as well - test results: 44 failed, 1738 passed, 23 skipped, 8 xfailed, 3 xpassed, 17070 warnings
- convert json encoders to field_serialiser decorators for variables and replace .dict/.json with model_dump and model_dump_json - change __private_attribute_values__ query over to use __pydantic_private__ - tests now only fail due to weird inheritence issue when attempting to pass session into either FuncTask or Task - Not sure whats causing this issue - Tried to get arround it by setting a ClassVar in the test but this then doesn't pass the session variable into values in the validation decorators - test results at: 18 failed, 1764 passed, 23 skipped
…nd of class inheritance with the variable - setting the value as None removed from RedBase - Still same issue with inheritence present within pydantic
|
Thanks a lot for making this PR! I highly appreciate it! I'm sorry for taking such a long time to get to work on this. I felt a bit overwhelmed to get back due to the work I know what's ahead to get back to speed, and my machine doesn't run VS Code anymore so I need to upgrade that hopefully during the next week. But maybe it's time to change my habbits. |
rocketry/session.py
Outdated
| tasks_as_daemon: bool = True | ||
| restarting: str = 'replace' | ||
| instant_shutdown: bool = False | ||
|
|
||
| timeout: datetime.timedelta = datetime.timedelta(minutes=30) | ||
| shut_cond: Optional['BaseCondition'] = None | ||
| cls_lock: Type = threading.Lock | ||
| cls_lock: threading.Lock = threading.Lock |
There was a problem hiding this comment.
I think semantically this is a Type (should be lock like type, perhaps in the future we allow other types of locking to support parallel scheduling instances). Or were there perhaps problems with this line?
There was a problem hiding this comment.
This definitely should be Type imo, but when I try and execute the code I seem to get a validation error when setting cls_lock:
Exception has occurred: ValidationError
1 validation error for Config
cls_lock
Input should be a type [type=is_type, input_value=<built-in function allocate_lock>, input_type=builtin_function_or_method]
Type[threading.Lock] seems to throw a similar error. The typing I've set here isn't ideal and still should be changed though. I've changed the type to be Callable since from the error in thinks I'm passing in a function type if that's okay?
rocketry/tasks/func.py
Outdated
| @@ -173,7 +179,7 @@ def __init__(self, func=None, **kwargs): | |||
| # We initiate the class lazily by creating | |||
| # almost empty shell class that is populated | |||
| # in next __call__ (which should occur immediately) | |||
| self._delayed_kwargs = kwargs | |||
| FuncTask._delayed_kwargs = kwargs | |||
There was a problem hiding this comment.
The same applies here. I think these should be instance attributes. It probably works this way but only if one does nothing out of the ordinary before setting the func (which is surprisingly common for users).
There was a problem hiding this comment.
This is a tricky one, I hated this solution but it was the only way I could get this to work.
For some background, I'm not the most knowledgable when it comes to pydantic so I'm not completely sure how this use to be, but one of the most recent migrations is this to Model config:
To instead preserve any extra data provided, you can set extra='allow'. The extra fields will then be stored in
BaseModel.__pydantic_extra__
extra="allow" has been set in the Task config to allow for this. But its mainly installing attributes that are unknown in __pydantic_extra__, I'm not sure if this functionality is new or not.
If I set those values back to self I get the following error:
Exception has occurred: AttributeError
'FuncTask' object has no attribute '__pydantic_extra__'
File "/home/jypo/rocketry-pydanticv2/rocketry/rocketry/tasks/func.py", line 182, in __init__
self._delayed_kwargs = kwargs
File "/home/jypo/rocketry-pydanticv2/rocketry/rocketry/session.py", line 420, in create_task
return FuncTask(name_include_module=False, _name_template='{func_name}', session=self, **kwargs)
File "/home/jypo/rocketry-pydanticv2/rocketry/rocketry/application.py", line 20, in task
return self.session.create_task(start_cond=start_cond, name=name, **kwargs)
File "/home/jypo/rocketry-pydanticv2/rocketry/test.py", line 28, in <module>
@app.task("every 3 seconds")
AttributeError: 'FuncTask' object has no attribute '__pydantic_extra__'
The odd part here is that _delay_kwargs is set in the class and it also has a default value, so why its trying to treat this as an "extra" variable I have no idea.
Additionally, I'm not completely sure if its the order of execution of the code, but when I run a debugger to try and see if __pydantic_extra__ does exist in the class it seems to have a traceback at this point.
The only way around this is to store delayed_kwargs inside the un-initated class variable (which is a bit shit).
I'll try and take another look at this one and see if I can come up with a better solution.
There was a problem hiding this comment.
So this has taken a bit of reverse engineering on the pydantic part. But this issue is to do with pydantic some kind of order of execution or something along those lines.
So how I best understand it. When we use the @app.task decorator it returns a FuncTask
FuncTask inherits from Task which inherits from BaseModel (FuncTask > Task > BaseModel).
Because both FuncTask and Task have overridden __init__ for the pydantic BaseModel (until super().__init__() is run) it doesn't fully initiate (which is why we see tracebacks in the model) while its executing FuncTasks __init__
The variables do partially exist (I'm presuming because of the validation being run before the init partially initiating a pydantic BaseModel?). So essentially, it looks like its trying to use pydantic functionality without it being fully initated (I think)
Horrible workaround
I tried calling model_construct to try and rebuild self using the BaseModel init but most solutions I tried broke the class in some form by overwriting existing variables
So I had a look at the pydantic code and mainly the way that the "model construct" function worked.
It has set a global variable (of a function) inside main _object_setattr and during the construct it calls the two following lines:
#inside a couple if statements
_object_setattr(m, '__pydantic_private__', None)
_object_setattr(m, '__pydantic_extra__', _extra)
I imported this functionality and set those manually within the FuncTask __init__ function and it seems to work. But its perhaps an awful solution and clearly not intended to be used.
from pydantic.main import _object_setattr
...
...
class FuncTask(Task):
...
...
def __init__(self, func=None, **kwargs):
only_func_set = func is not None and not kwargs
no_func_set = func is None and kwargs.get('path') is None
_object_setattr(self, "__pydantic_extra__", {})
_object_setattr(self, "__pydantic_private__", None)
if no_func_set:
# FuncTask was probably called like:
# @FuncTask(...)
# def myfunc(...): ...
# We initiate the class lazily by creating
# almost empty shell class that is populated
# in next __call__ (which should occur immediately)
self._delayed_kwargs = kwargs
return
if only_func_set:
...
This isn't a very elegant solution, but it looks like the best that can be done without involving a massive rework of some of this functionality with delayed_kwargs?
What would be your opinion on this? I'll commit these changes for now, but can easily revert if needed.
rocketry/tasks/func.py
Outdated
| @@ -192,7 +199,7 @@ def __call__(self, *args, **kwargs): | |||
| func = args[0] | |||
| super().__init__(func=func, **self._delayed_kwargs) | |||
| self._set_descr(is_delayed=False) | |||
| self._delayed_kwargs = {} | |||
| FuncTask._delayed_kwargs = {} | |||
rocketry/core/task.py
Outdated
|
|
||
| # Class | ||
| permanent: bool = False # Whether the task is not meant to finish (Ie. RestAPI) | ||
| _actions: ClassVar[Tuple] = ("run", "fail", "success", "inaction", "terminate", None, "crash") | ||
| fmt_log_message: str = r"Task '{task}' status: '{action}'" | ||
|
|
||
| daemon: Optional[bool] | ||
| daemon: Optional[bool] = False |
There was a problem hiding this comment.
If I recall correctly, if this is not set (should be None), this is fetched from the session configs thus setting default here doesn't allow us to see that the user didn't want to specify the daemon attr for this particular task (thus should be gotten from the config).
There was a problem hiding this comment.
I did have issues with this field as part of the migration as well. If this field is not set, then during the __init__ for Task it would raise a validation error for daemon when calling super().__init__(**kwargs).
Exception has occurred: ValidationError
1 validation error for FuncTask
daemon
Field required [type=missing, input_value={'func': <function do_thi...s', 'name': 'do_things'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.0.3/v/missing
Noting as well that this validation error is coming from FuncTask rather than task so I'm not sure if this is some inheritance weirdness again.
Its strange with the field clearly being stated as an Optional[bool] in the class. Would that functionality still work as intended when setting the field with a default value of None instead of False? That still seems to allow the code to run. Or would that still cause the same issues with not inheriting that attribute from Config?
There was a problem hiding this comment.
I see what you mean on this now
def run_as_process(self, params:Parameters, direct_params:Parameters, task_run:TaskRun, daemon=None, log_queue: multiprocessing.Queue=None):
"""Create a new process and run the task on that."""
session = self.session
params = params.pre_materialize(task=self, session=session)
direct_params = direct_params.pre_materialize(task=self, session=session)
# Daemon resolution: task.daemon >> scheduler.tasks_as_daemon
log_queue = session.scheduler._log_queue if log_queue is None else log_queue
daemon = self.daemon if self.daemon is not None else session.config.tasks_as_daemon
Hopefully with a default value of None this should work now rather than False.
|
By the way, the CI crashed on: I guess this is what you meant that we need to change Redbird as well. I'll take a look into it tomorrow (hopefully have time in the evening). |
It's no problem, I'm a big fan of this module so more than happy to try and help. You're correct the CI tests fail due to the dependancy on RedBird, I've posted the summary of the tests below so you can see where its up to when run locally. More than happy to help with a PR on RedBird as well if that would help! I've tried my best to migrate this overall but I'm running into an issue with the last couple of tests failing due to the nature in which pydantic has changed shadowing attributes during class inheritance. I believe its this bit of functionality here Now for each attribute it checks to see if it exists in the base class and raises an error if it does. You can get round this by setting ClassVar, which is why you're seeing so much ClassVar as types. I'll try and take another look at it to see if there is maybe another way around this. So apart from the last couple of tests this seems to have fixed the code enough to run, but the tests fail if you directly try to create a DummyTask that inherits Task. So far I haven't managed to find a way around this, but I'm still trying! Thanks so much for reviewing the changes, I'll definitely take a look at those when I get chance and push some changes. Heres where I'm up to overall (I believe the warnings you see are for the most part depreciation warnings from RedBird. Any in Rocketry I still need to review once I have all the tests running): |
- Remove brackes from RedBase, base function - Set cls_lock attribute to callable (Waiting for clarification) - validate_default in model config no longer necessary after testing - Changed daemon attribute from False to Nonw (Waiting for clarification) - _delayed_kwargs no longer a task variable
…es to allow setting of self when setting _delayed_kwargs:O
|
Sorry, had something unexpected yesterday and now three days of hobbies. I'll try to continue on Friday, or Saturday at the latest. |
- in tests replace .dict statment with .model_dump - tidy up inport of _object_setter - Add setting (temporarily) into tox.ini to ignore deprecation warnings from redbird
|
Just adding a bit of an update on an issue I've been having with this. Theres one issue that's been causing me a massive headache. I'm going to be honest and say I've spent atleast 10+ hours on this issue! haha. BackgroundA couple of the tests that create a class (called DummyTask) which inherits From testing this in a separate file I found no matter what I did, I could never get this type of inheritance working. I did try in dummy test setting "session" as a ClassVar to ignore this validation. It partially worked and started to initiate the task inheritance. but as soon as Task called So the train of thought I had today was how could FuncTask inherit Task and pass I actually ended up cloning FuncTask into a different file and tested it locally and got the inheritance error. However, as soon as I added an import to I've looked all over the application and I can't for the life of me figure out why I stop getting the inheritance error as soon as I add an import statement in an I don't really know what I'm asking here, but have you got anything running in this application which is looking for through all import statements and doing some kind of validation? lol The only thing remotely close is the _setup.py file. But I don't have to add the function into the forwardrefs replacement to get this to work (I only have to do that to get it to build the function correctly etc). Proposed solutionSo as stated around 19 tests are failing when trying to create a DummyTask. This isn't affecting the application itself but it only causing issues in tests. The solution I've found is to create a new file in I then add it to the I then need to issue model_rebuild (forward_refs) as part of the Now in the test files that used a DummyTask we replace it with an import statement. instead of putting the class in the test This then starts to pass the tests So basically we are moving the dummy inside the application rather than in the tests files. I'll push this change complete at some point tomorrow, but if you're not happy with that solution just let me know. |
- Implemented fix to DummyTask by having it as part of the application preventing inheritence error. - Added dummy task to __init__.py for rocketry.tasks and into the setup for ForwardRefs - Fixed test_meta by removing the classVar previously put in for the _FuncCondWrapper function - Some general tidy ups Test results at: 1 failed, 1781 passed, 23 skipped, 8 xfailed, 3 xpassed, 8707 warnings in 41.18s - The one failed test is likely to do with the way I'm using Virtualenv, as this is also present in the stable release of Rocketry on my machine
…andler removed temp pytest to ignore redbird warnings:
|
With the solution implemented above, I have gone and ahead and took this PR out of draft. Results of test: The test_build fails because I think its some kind of weirdness with my virtualenv. The same test fails if I run the with the current main and pydantic 1.10.10 installed. I'm pretty certain the vast majority of warnings are all coming from redbird. I couldn't find a way to exclude any warnings that come from redbird in pytest. But if I have missed something, then I would be happy to create another PR to address the rest of the deprecation warnings post redbird. I'll keep an eye on redbirds progress, but if there isn't much shift with migrating it to v2 I'll create a PR for it. When you get time to address some of the comments and the rest of the changes let me know and I can find alternate solutions if I have to. |
|
Any updates here? |
@DenysMoskalenko These PRs also break pydanticv1 support entirely, not sure if that would affect existing projects out there and versioning may need to be applied and supported. Happy to give anyone access to the RedBird PR Repo if they have time to chip in. |
- Not sure what changed but pickling stopped working again (maybe a recent update to pydantic again) - To resolve this had to add 'returns' to the pickled attributes set to None - This seems to have fixed the initial issues but has caused some tests to fail - Will begin debugging this again
…post materilising seems to fail. Not sure if this is no longer possible in the new version
|
Any updates? Thanks for the initiative to add support for Pydantic 2, btw! |
Haven't looked at this for a bit. Last time I worked on this I was having some issues with pickleing (So some of the multiprocessing aspects of the tasks were not working (I think! some stuff worked with basic testing, but some of the tests still fail)) I had the pickleing working on an Ubuntu machine, but am having an issue on MacOS which is weird. Single thread, async, and Multi-threaded all pass the unit tests. Apart from that, the rest was fine in initial testing. So I guess it's just whenever Miksus has time to get to this really. I'm pretty much ready to work on any changes that need to be made once reviewed again. |
|
@Miksus Are you able to look at this soon? I like Rocketry, but being stuck on pydantic v1 is holding me back. I am happy to contribute to getting this over the line, but based on comments above this PR needs your love |
|
I normally do not do +1 posts. but I am making an exception here 🤓 |
Attempting to start work on the PydanticV2 migration.
Changes made so far:
Pretty much all the tests still fail and this needs heavily reviewing. I'll continue to work on this PR to assist.
As it stands with this version of the code a basic hello_world task can be run.
This is with PydanticV2 (2.0.3) and installing ManiMozaffar red-bird branch which has quick fixes to append .v1 to the pydantic imports.
Will continue to work on making tests pass with v2 installed.