Skip to content

Conversation

slafs
Copy link
Contributor

@slafs slafs commented Jul 5, 2023

Context

It seems like the equivalent of PyObject from v1
is the ImportString type in v2,
but there's no mention of it in the migration docs.

Change Summary

Add pydantic.PyObject -> pydantic.ImportString row
in the "Moved in Pydantic V2" table.

Open questions

  • There's a "known limitation" note in the ImportString docs,
    not sure if I should mention it in the migration doc.
  • Also, not sure if I should update pydantic/_migration.py too 🤔.

Notes

BTW the doc also states:

This is actively being worked on.

can this be tracked somewhere?

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

slafs and others added 2 commits July 5, 2023 15:47
It seems like the equivalent of PyObject from v1
is the ImportString type in v2,
but there's no mention of it in the migration docs.

There's a "known limitation" note in the ImportString docs,
not sure if I should mention it in the migration doc.

BTW the doc also states:
> This is actively being worked on.

can this be tracked somewhere?

Also, not sure if I should update `pydantic/_migration.py` too 🤔.
@dmontagu
Copy link
Contributor

dmontagu commented Jul 5, 2023

I think there are enough differences in the two types that I'm not sure it makes sense to document it as a pure move, but I do think it's worth adding a note to the migration docs about it.

I pushed this change to this PR, if you disagree with it we can revert, otherwise I'm happy to merge this.

@Kludex
Copy link
Member

Kludex commented Jul 5, 2023

Do I need to add in bump-pydantic?

@slafs
Copy link
Contributor Author

slafs commented Jul 5, 2023

@dmontagu thanks, but wouldn't it make sense to still have the row in "Moved in Pydantic V2" table, but this time link to the note you added in the same migration doc?

@slafs
Copy link
Contributor Author

slafs commented Jul 5, 2023

there are enough differences in the two types

Interesting. Apart from the default value limitation, what else do you see as different between the two?

@slafs
Copy link
Contributor Author

slafs commented Jul 5, 2023

Either way, I'm fine with the current version of this PR 👍.

@slafs
Copy link
Contributor Author

slafs commented Jul 5, 2023

Do I need to add in bump-pydantic?

@Kludex that would be nice, but IMHO when there was a default value for PyObject prop,
then a TODO comment should be added (because from what I understand there's no workaround for this limitation ATM).

@dmontagu
Copy link
Contributor

dmontagu commented Jul 5, 2023

Okay, actually, it's more similar than I thought (I thought it would require the input to validation be a string, but it doesn't).

There are a handful of improvements, which isn't the case for most of the other things in those tables, but I don't necessarily think that means it shouldn't be included in the table. For example:

  • Now .model_dump_json() works, whereas in v1 with PyObject, .json() didn't.
  • ImportString functions like a generic type, and can apply constraints:
from pydantic import BaseModel, ImportString


class A(BaseModel):
    x: ImportString[type[int]]


A(x='math.cos')
"""
pydantic_core._pydantic_core.ValidationError: 1 validation error for A
x
  Input should be a subclass of int [type=is_subclass_of, input_value=<built-in function cos>, input_type=builtin_function_or_method]
"""

The biggest breaking change is that ImportString no longer forces validate_default the way PyObject did, so the following code wouldn't behave as expected:

    class ImportStringModel(BaseModel):
        module: ImportString = 'os.path'

    m = ImportStringModel()
    assert m.module == os.path  # this will fail, as module will be the string literal `'os.path'`

I don't know if there is an easy way to fix that though without changes to ImportString. One thing I think we could do, if nothing else, is expose pydantic._internal._validators.import_string as a staticmethod on ImportString, which would let you do

module = ImportString.import_string('os.path')  # equivalent to `module = os.path`

But I guess that's orthogonal to this PR.

Not quite sure how to proceed, @Kludex thoughts?

@slafs
Copy link
Contributor Author

slafs commented Jul 5, 2023

  • Now .model_dump_json() works, whereas in v1 with PyObject, .json() didn't.
  • ImportString functions like a generic type, and can apply constraints:

Right, so from the PyObject -> ImportString migration perspective this is still fine, right? In other words you could say there's nothing PyObject could do that ImportString can't (modulo the default value issue).

The biggest breaking change is that ImportString no longer forces validate_default the way PyObject did

Right, and this is documented. Hence my Notes section in this PR description (the docs ATM say "This is actively being worked on" 😅).

But I guess that's orthogonal to this PR.

I agree.

@dmontagu
Copy link
Contributor

dmontagu commented Jul 5, 2023

Okay, I think it makes sense to undo my docs changes then (which I have done, and restored yours), but I did add a change to pydantic._migration to also handle the import change automatically (with a deprecation warning).

@Kludex I think we should therefore address this in bump pydantic.

@dmontagu dmontagu merged commit d2a002b into pydantic:main Jul 5, 2023
@Kludex
Copy link
Member

Kludex commented Jul 5, 2023

I'll implement it there. 👍

@slafs slafs deleted the patch-1 branch July 6, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants