Add an optional arg `deadline` to `dev.deprecated` to raise warning after deadline #631

DanielYang59 · 2024-02-27T05:00:28Z

Major changes:

Add an optional arg deadline for declaration of the deadline, and raise a CI warning pass that date
(thanks @janosh for the helpful discussion in Deprecate _parse_atomic_densities in BaderAnalysis and fix Bader test setup materialsproject/pymatgen#3656 (comment))
Add type hints

Minor tweaks:

Remove unused deps in requirements-ci.txt (let me know if they're intended otherwise)
Minor clean up of test_dev unit test file (removed a random Class A unused anywhere)
Replace deprecated general_plain_validator_function from monty/json.py

Side notes:

running pytest on test_tempfile.py seems to modify test_files/3000_lines.txt.gz in place, this should not happen.

DanielYang59 · 2024-02-27T06:15:39Z

requirements-ci.txt

 pydantic
-flake8


Let me know if they're still needed anywhere. Thanks!

monty/dev.py

DanielYang59 · 2024-02-27T08:56:37Z

docs/monty.dev.md

-user/real as output by time(1) when called with an optimally scaling
-userspace-only program. Return -1 if ncpus cannot be detected. Taken from:
-[http://stackoverflow.com/questions/1006289/how-to-find-out-the-number-of](http://stackoverflow.com/questions/1006289/how-to-find-out-the-number-of)-
-cpus-in-python


Is the docs automatically generated? It seems this method doesn't exist anymore.

yes, the docs are auto-generated from doc strings. possible that this needs to be rerun @shyuep?

Thanks for the input.

DanielYang59 · 2024-02-27T09:02:08Z

monty/json.py

@@ -328,7 +328,7 @@ def __get_pydantic_core_schema__(cls, source_type, handler):
        if core_schema is None:
            raise RuntimeError("Pydantic >= 2.0 is required for validation")

-        s = core_schema.general_plain_validator_function(cls.validate_monty_v2)
+        s = core_schema.with_info_plain_validator_function(cls.validate_monty_v2)


Replaced from:

test_json.py::TestJson::test_pydantic_integrations /home/yang/Developers/monty/monty/json.py:331: DeprecationWarning: `general_plain_validator_function` is deprecated, use `with_info_plain_validator_function` instead. s = core_schema.general_plain_validator_function(cls.validate_monty_v2)

Should fix #598.

DanielYang59 · 2024-03-01T12:19:04Z

Should I learn something from your PR? Maybe use a warning for "softer" remainder (or let user/developer pass a Warning or Error instance?)

janosh · 2024-03-01T12:33:05Z

yeah, i think a warning is worth considering. but we definitely also need a way to make the warning only show up in the repo's own CI, not in that of downstream packages. that's why i added

if os.getenv("GITHUB_REPOSITORY") == "materialsproject/pymatgen"

but it would be quite clunky having to pass in the repo owner and name for every deprecation removal reminder. not sure what the best solution is yet

DanielYang59 · 2024-03-07T02:43:01Z

yeah, i think a warning is worth considering.

I was being stupid. I realize it's already DeprecationWarning not DeprecationError.

but we definitely also need a way to make the warning only show up in the repo's own CI, not in that of downstream packages.

I just came up with a "not so clean" solution but assume it should work. Maybe just nest a git config --get remote.origin.url command inside the decorator and get the code's own repo name like git@github.com:DanielYang59/monty.git, and compare it with GITHUB_REPOSITORY.

But I'm a little confused myself which url this command would return if it's imported by a downstream package, also this may very likely fail if someone change origin to another name (But I think we're very close, if the name is not origin, we could still get it with git remote -v). But would this be too "geeky"? What do you think? @janosh

janosh · 2024-03-07T07:18:54Z

using subprocess to run git remote -v makes the assumption that git is installed. but that could be fine. we just have to do nothing if git is missing. the overarching principle is not to warn unless we're certain we're running in the origin repo's own CI. so unless all criteria are met, we just do nothing

monty/dev.py

janosh

very nice tests! this looks ready to go after addressing 2 minor comments. @shyuep can you review and/or merge?

tests/test_dev.py

monty/dev.py

DanielYang59 · 2024-03-11T06:01:56Z

Noticed another test failure seemingly related pydantic (I assume it's not related to this PR):

Error Message

============================================ test session starts =============================================
platform darwin -- Python 3.9.6, pytest-8.0.2, pluggy-1.4.0
rootdir: /Users/yang/developer/monty
plugins: cov-4.1.0, xdist-3.5.0
collected 23 items                                                                                            

tests/test_json.py ....................F..                                                              [100%]

================================================== FAILURES ===================================================
_____________________________________ TestJson.test_pydantic_integrations _____________________________________

value = ForwardRef('LimitedMSONClass | dict')
globalns = {'@py_builtins': <module 'builtins' (built-in)>, '@pytest_ar': <module '_pytest.assertion.rewrite' from '/Users/yang/d...<class 'pydantic.main.BaseModel'>, 'ClassContainingDataFrame': <class 'tests.test_json.ClassContainingDataFrame'>, ...}
localns = {'__abstractmethods__': frozenset(), '__annotations__': {'a': 'LimitedMSONClass | dict'}, '__class_vars__': set(), '__doc__': None, ...}

    def eval_type_backport(
        value: Any, globalns: dict[str, Any] | None = None, localns: dict[str, Any] | None = None
    ) -> Any:
        """Like `typing._eval_type`, but falls back to the `eval_type_backport` package if it's
        installed to let older Python versions use newer typing features.
        Specifically, this transforms `X | Y` into `typing.Union[X, Y]`
        and `list[X]` into `typing.List[X]` etc. (for all the types made generic in PEP 585)
        if the original syntax is not supported in the current Python version.
        """
        try:
>           return typing._eval_type(  # type: ignore
                value, globalns, localns
            )

venv/lib/python3.9/site-packages/pydantic/_internal/_typing_extra.py:240: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/typing.py:290: in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = ForwardRef('LimitedMSONClass | dict')
globalns = {'@py_builtins': <module 'builtins' (built-in)>, '@pytest_ar': <module '_pytest.assertion.rewrite' from '/Users/yang/d...<class 'pydantic.main.BaseModel'>, 'ClassContainingDataFrame': <class 'tests.test_json.ClassContainingDataFrame'>, ...}
localns = {'__abstractmethods__': frozenset(), '__annotations__': {'a': 'LimitedMSONClass | dict'}, '__class_vars__': set(), '__doc__': None, ...}
recursive_guard = frozenset()

    def _evaluate(self, globalns, localns, recursive_guard):
        if self.__forward_arg__ in recursive_guard:
            return self
        if not self.__forward_evaluated__ or localns is not globalns:
            if globalns is None and localns is None:
                globalns = localns = {}
            elif globalns is None:
                globalns = localns
            elif localns is None:
                localns = globalns
            type_ =_type_check(
>               eval(self.__forward_code__, globalns, localns),
                "Forward references must evaluate to types.",
                is_argument=self.__forward_is_argument__,
            )
E           TypeError: unsupported operand type(s) for |: 'type' and 'type'

/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/typing.py:546: TypeError

The above exception was the direct cause of the following exception:

self = <tests.test_json.TestJson object at 0x13f882f40>

    def test_pydantic_integrations(self):
        from pydantic import BaseModel, ValidationError
    
        global ModelWithMSONable  # allow model to be deserialized in test
        global LimitedMSONClass
    
        class ModelWithMSONable(BaseModel):
            a: GoodMSONClass
    
        test_object = ModelWithMSONable(a=GoodMSONClass(1, 1, 1))
        test_dict_object = ModelWithMSONable(a=test_object.a.as_dict())
        assert test_dict_object.a.a == test_object.a.a
        dict_no_class = test_object.a.as_dict()
        dict_no_class.pop("@class")
        dict_no_class.pop("@module")
        test_dict_object = ModelWithMSONable(a=dict_no_class)
        assert test_dict_object.a.a == test_object.a.a
    
        assert test_object.model_json_schema() == {
            "title": "ModelWithMSONable",
            "type": "object",
            "properties": {
                "a": {
                    "title": "A",
                    "type": "object",
                    "properties": {
                        "@class": {"enum": ["GoodMSONClass"], "type": "string"},
                        "@module": {"enum": ["tests.test_json"], "type": "string"},
                        "@version": {"type": "string"},
                    },
                    "required": ["@class", "@module"],
                }
            },
            "required": ["a"],
        }
    
        d = jsanitize(test_object, strict=True, enum_values=True, allow_bson=True)
        assert d == {
            "a": {
                "@module": "tests.test_json",
                "@class": "GoodMSONClass",
                "@version": "0.1",
                "a": 1,
                "b": 1,
                "c": 1,
                "d": 1,
                "values": [],
            },
            "@module": "tests.test_json",
            "@class": "ModelWithMSONable",
            "@version": "0.1",
        }
        obj = MontyDecoder().process_decoded(d)
        assert isinstance(obj, BaseModel)
        assert isinstance(obj.a, GoodMSONClass)
        assert obj.a.b == 1
    
        # check that a model that is not validated by pydantic still
        # gets completely deserialized.
        global ModelWithDict  # allow model to be deserialized in test
    
        class ModelWithDict(BaseModel):
            a: dict
    
        test_object_with_dict = ModelWithDict(a={"x": GoodMSONClass(1, 1, 1)})
        d = jsanitize(
            test_object_with_dict, strict=True, enum_values=True, allow_bson=True
        )
        assert d == {
            "a": {
                "x": {
                    "@module": "tests.test_json",
                    "@class": "GoodMSONClass",
                    "@version": "0.1",
                    "a": 1,
                    "b": 1,
                    "c": 1,
                    "d": 1,
                    "values": [],
                }
            },
            "@module": "tests.test_json",
            "@class": "ModelWithDict",
            "@version": "0.1",
        }
        obj = MontyDecoder().process_decoded(d)
        assert isinstance(obj, BaseModel)
        assert isinstance(obj.a["x"], GoodMSONClass)
        assert obj.a["x"].b == 1
    
        # check that if an MSONable object raises an exception during
        # the model validation it is properly handled by pydantic
        global ModelWithUnion  # allow model to be deserialized in test
        global ModelWithLimited  # allow model to be deserialized in test
    
        class ModelWithLimited(BaseModel):
            a: LimitedMSONClass
    
>       class ModelWithUnion(BaseModel):

tests/test_json.py:827: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
venv/lib/python3.9/site-packages/pydantic/_internal/_model_construction.py:178: in __new__
    set_model_fields(cls, bases, config_wrapper, types_namespace)
venv/lib/python3.9/site-packages/pydantic/_internal/_model_construction.py:452: in set_model_fields
    fields, class_vars = collect_model_fields(cls, bases, config_wrapper, types_namespace, typevars_map=typevars_map)
venv/lib/python3.9/site-packages/pydantic/_internal/_fields.py:122: in collect_model_fields
    type_hints = get_cls_type_hints_lenient(cls, types_namespace)
venv/lib/python3.9/site-packages/pydantic/_internal/_typing_extra.py:212: in get_cls_type_hints_lenient
    hints[name] = eval_type_lenient(value, globalns, localns)
venv/lib/python3.9/site-packages/pydantic/_internal/_typing_extra.py:224: in eval_type_lenient
    return eval_type_backport(value, globalns, localns)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

value = ForwardRef('LimitedMSONClass | dict')
globalns = {'@py_builtins': <module 'builtins' (built-in)>, '@pytest_ar': <module '_pytest.assertion.rewrite' from '/Users/yang/d...<class 'pydantic.main.BaseModel'>, 'ClassContainingDataFrame': <class 'tests.test_json.ClassContainingDataFrame'>, ...}
localns = {'__abstractmethods__': frozenset(), '__annotations__': {'a': 'LimitedMSONClass | dict'}, '__class_vars__': set(), '__doc__': None, ...}

    def eval_type_backport(
        value: Any, globalns: dict[str, Any] | None = None, localns: dict[str, Any] | None = None
    ) -> Any:
        """Like `typing._eval_type`, but falls back to the `eval_type_backport` package if it's
        installed to let older Python versions use newer typing features.
        Specifically, this transforms `X | Y` into `typing.Union[X, Y]`
        and `list[X]` into `typing.List[X]` etc. (for all the types made generic in PEP 585)
        if the original syntax is not supported in the current Python version.
        """
        try:
            return typing._eval_type(  # type: ignore
                value, globalns, localns
            )
        except TypeError as e:
            if not (isinstance(value, typing.ForwardRef) and is_backport_fixable_error(e)):
                raise
            try:
                from eval_type_backport import eval_type_backport
            except ImportError:
>               raise TypeError(
                    f'You have a type annotation {value.__forward_arg__!r} '
                    f'which makes use of newer typing features than are supported in your version of Python. '
                    f'To handle this error, you should either remove the use of new syntax '
                    f'or install the `eval_type_backport` package.'
                ) from e
E               TypeError: You have a type annotation 'LimitedMSONClass | dict' which makes use of newer typing features than are supported in your version of Python. To handle this error, you should either remove the use of new syntax or install the `eval_type_backport` package.

venv/lib/python3.9/site-packages/pydantic/_internal/_typing_extra.py:249: TypeError
============================================== warnings summary ===============================================
tests/test_json.py::TestJson::test_pydantic_integrations
tests/test_json.py::TestJson::test_pydantic_integrations
  /Users/yang/developer/monty/venv/lib/python3.9/site-packages/pydantic/main.py:1024: PydanticDeprecatedSince20: The `dict` method is deprecated; use `model_dump` instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.6/migration/
    warnings.warn('The `dict` method is deprecated; use `model_dump` instead.', category=PydanticDeprecatedSince20)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================================== short test summary info ===========================================
FAILED tests/test_json.py::TestJson::test_pydantic_integrations - TypeError: You have a type annotation 'LimitedMSONClass | dict' which makes use of newer typing features t...
================================== 1 failed, 22 passed, 2 warnings in 1.02s ===================================

janosh · 2024-03-11T06:07:45Z

not sure why this is popping up now and not before but you need to replace

monty/tests/test_json.py

Line 828 in 1a91828

a: LimitedMSONClass | dict

with

Union[LimitedMSONClass, dict]

DanielYang59 · 2024-03-11T06:19:49Z

not sure why this is popping up now and not before but you need to replace

monty/tests/test_json.py

Line 828 in 1a91828

a: LimitedMSONClass | dict

with
Union[LimitedMSONClass, dict]

Yes it's odd. Not sure what is happening as this PR didn't touch anything about the Union operator. This operator | should be introduced in Python 3.10.

And I assume the Python version requirement has to be updated too:

monty/pyproject.toml

Line 12 in 4d60fd4

requires-python = ">=3.9"

And from the main README:

monty/README.md

Line 27 in 4d60fd4

Monty supports Python 3.x.

This is a bit misleading.

janosh · 2024-03-11T06:24:01Z

This operator | should be introduced in Python 3.10.

it's a pydantic restriction which doesn't support forward references below 3.10. see pydantic/pydantic#3300. so you can't use pipe for type unions in any file that defines a pydantic schema

tests/test_json.py

janosh · 2024-03-15T04:33:15Z

@shyuep could you review?

janosh · 2024-03-23T05:46:10Z

@shyuep ping

DanielYang59 added 8 commits February 27, 2024 10:40

add type hints

36131f7

add deprecation deadline

05d77e0

remove unused deps

a51fb8d

replace deprecated func

2956082

remove unused class def

a39ca06

tweak test namings

4bdc371

make always warn global

2088f8d

add test for deprecation deadline

cb91417

DanielYang59 commented Feb 27, 2024

View reviewed changes

requirements-ci.txt

pydantic

flake8

Copy link

Contributor Author

DanielYang59 Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if they're still needed anywhere. Thanks!

janosh suggested changes Feb 27, 2024

View reviewed changes

monty/dev.py Outdated Show resolved Hide resolved

monty/dev.py Outdated Show resolved Hide resolved

monty/dev.py Outdated Show resolved Hide resolved

DanielYang59 added 4 commits February 27, 2024 16:27

remove non-existence get_ncpus method

e97cfe9

adjust warning type and message

783a982

use stricter "os.getenv("CI")"

d5b463b

use int tuple as deadline

5cca0dc

DanielYang59 commented Feb 27, 2024

View reviewed changes

tweak docstring

2901b9d

DanielYang59 commented Feb 27, 2024

View reviewed changes

DanielYang59 marked this pull request as draft March 4, 2024 13:12

DanielYang59 changed the title ~~Add an optional arg deadline to dev.deprecated to raise error after deadline~~ Add an optional arg deadline to dev.deprecated to raise warning after deadline Mar 7, 2024

fix naming with "warning"

bb4af84

add test for code owner

59458a2

janosh suggested changes Mar 8, 2024

View reviewed changes

monty/dev.py Show resolved Hide resolved

DanielYang59 added 3 commits March 8, 2024 17:57

add lstrip for https clone

a37e12c

revise warn method name

0a13730

add no warn tests

a247764

DanielYang59 marked this pull request as ready for review March 11, 2024 03:45

DanielYang59 requested a review from janosh March 11, 2024 03:46

janosh approved these changes Mar 11, 2024

View reviewed changes

tests/test_dev.py Outdated Show resolved Hide resolved

monty/dev.py Outdated Show resolved Hide resolved

DanielYang59 added 2 commits March 11, 2024 13:57

replace single letter var

247b297

remove unnecessary date convert func

5d62012

replace | operator with Union

80bfea5

DanielYang59 commented Mar 11, 2024

View reviewed changes

tests/test_json.py Show resolved Hide resolved

convert comment to docstring

de31119

shyuep merged commit 9ecdf1b into materialsvirtuallab:master Mar 23, 2024
1 check passed

DanielYang59 deleted the dep-date branch March 24, 2024 11:50

DanielYang59 mentioned this pull request Mar 27, 2024

[Bug]: DeprecationWarning due to use of general_plain_validator_function in MSONable.__get_pydantic_core_schema__ with Pydantic v2 #598

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an optional arg `deadline` to `dev.deprecated` to raise warning after deadline #631

Add an optional arg `deadline` to `dev.deprecated` to raise warning after deadline #631

DanielYang59 commented Feb 27, 2024 •

edited

DanielYang59 Feb 27, 2024

DanielYang59 Feb 27, 2024

janosh Feb 27, 2024

DanielYang59 Feb 27, 2024

DanielYang59 Feb 27, 2024

DanielYang59 Mar 15, 2024

DanielYang59 commented Mar 1, 2024 •

edited

janosh commented Mar 1, 2024 •

edited

DanielYang59 commented Mar 7, 2024 •

edited

janosh commented Mar 7, 2024

janosh left a comment

DanielYang59 commented Mar 11, 2024 •

edited

janosh commented Mar 11, 2024 •

edited

DanielYang59 commented Mar 11, 2024

janosh commented Mar 11, 2024 •

edited

janosh commented Mar 15, 2024

janosh commented Mar 23, 2024

Add an optional arg deadline to dev.deprecated to raise warning after deadline #631

Add an optional arg deadline to dev.deprecated to raise warning after deadline #631

Conversation

DanielYang59 commented Feb 27, 2024 • edited

Major changes:

Minor tweaks:

Side notes:

DanielYang59 Feb 27, 2024

Choose a reason for hiding this comment

DanielYang59 Feb 27, 2024

Choose a reason for hiding this comment

janosh Feb 27, 2024

Choose a reason for hiding this comment

DanielYang59 Feb 27, 2024

Choose a reason for hiding this comment

DanielYang59 Feb 27, 2024

Choose a reason for hiding this comment

DanielYang59 Mar 15, 2024

Choose a reason for hiding this comment

DanielYang59 commented Mar 1, 2024 • edited

janosh commented Mar 1, 2024 • edited

DanielYang59 commented Mar 7, 2024 • edited

janosh commented Mar 7, 2024

janosh left a comment

Choose a reason for hiding this comment

DanielYang59 commented Mar 11, 2024 • edited

janosh commented Mar 11, 2024 • edited

DanielYang59 commented Mar 11, 2024

janosh commented Mar 11, 2024 • edited

janosh commented Mar 15, 2024

janosh commented Mar 23, 2024

Add an optional arg `deadline` to `dev.deprecated` to raise warning after deadline #631

Add an optional arg `deadline` to `dev.deprecated` to raise warning after deadline #631

DanielYang59 commented Feb 27, 2024 •

edited

DanielYang59 commented Mar 1, 2024 •

edited

janosh commented Mar 1, 2024 •

edited

DanielYang59 commented Mar 7, 2024 •

edited

DanielYang59 commented Mar 11, 2024 •

edited

janosh commented Mar 11, 2024 •

edited

janosh commented Mar 11, 2024 •

edited