Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.24%. Comparing base (8a968e4) to head (0d1090e).
Report is 63 commits behind head on develop.

@@           Coverage Diff            @@
##           develop     #538   +/-   ##
========================================
  Coverage    84.24%   84.24%           
========================================
  Files           26       26           
  Lines         8209     8209           
  Branches      1697     1702    +5     
========================================
  Hits          6916     6916           
  Misses        1293     1293           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lan496, @LecrisUT, I have no idea. Could you decide?

I have no concerns to merge.

@LecrisUT What do you think? Unfortunately, Some packages using spglib are slow in migration...

Specifically, I was using pytest and flooded by lines like below.

  /usr/local/lib/python3.11/site-packages/spglib/spglib.py:115: DeprecationWarning: dict interface (SpglibDataset['number']) is deprecated.Use attribute interface ({self.__class__.__name__}.{key}) instead

I think this is because pyxtal is using the old API.

I wrote ignore::DeprecationWarning:spglib.* in filterwarning section of pytest.ini to silence these for now, but in case I accidentally write some code using the old API, I want to set stacklevel as 2 and rewrite filterwarning as ignore::DeprecationWarning:pyxtal.

I believe DeprecationWarning generally should be emitted from where deprecated code is used rather than from deprecated code itself so I would like to set stacklevel to all of them, but if there's any concern I will follow your decision.

@ktns can you check on how to make sure the warning is emitted only once. That would be a reasonable thing to add at the very least. It seems that the intended filter did not work as I thought:

The deprecation in pyxtal can be seen here:
https://github.com/MaterSim/PyXtal/blob/e086219624181a1b9ac9c44aa291808a54887171/pyxtal/miscellaneous/gulp_c.py#L19-L22
where it should change to

         info = get_symmetry_dataset(s, symprec=1e-1)
         s.write("1.vasp", format="vasp", vasp5=True, direct=True)
         os.system("cat 1.vasp >> " + file)
-        print("{:4d} {:8.3f} {:s}".format(i, calc.energy / 8, info["international"]))
+        print("{:4d} {:8.3f} {:s}".format(i, calc.energy / 8, info.international))

If you could make a PR change there it would be awesome. Either bump the minimum spglib version and use the new interface, or add filterwarning scoped as specific as possible and add a tracker for when the interface would be changed.

I think why many warning lines are emitted, not once, is because warning messages include different keys.

PR to pyxtal is right way to go of course but I can't send PRs (and wait them to be merged) to every library I come across, so I would like a way to filter warnings per caller basis.

I think why many warning lines are emitted, not once, is because warning messages include different keys.

That should not be the case if I read the usage of module filter correct. Maybe you can change it to module+lineno instead of message filter, but that seems brittle. Another option could be to add a filterwanings("ignore") after

PR to pyxtal is right way to go of course but I can't send PRs (and wait them to be merged) to every library I come across, so I would like a way to filter warnings per caller basis.

Not sure I got what you mean. But I think a good design is to add the filterwarning as close to the code you have control in this case.

Overall if I read the stacklevel part correctly, setting it to 2 would mean that the user of pyxtal will get the warning, but not pyxtal project itself, which would be really bad wouldn't it? Did I read that correctly?

That should not be the case if I read the usage of module filter correct. Maybe you can change it to module+lineno instead of message filter, but that seems brittle.

"module" filter suppresses only warnings with identical message and category.

import warnings
warnings.filterwarnings("module", category=UserWarning)

for i in range(10):
  warnings.warn(f"i={i}", UserWarning)
for i in range(10):
  warnings.warn(f"i={i}", UserWarning)

will emit 10 warning lines.

Overall if I read the stacklevel part correctly, setting it to 2 would mean that the user of pyxtal will get the warning, but not pyxtal project itself, which would be really bad wouldn't it? Did I read that correctly?

I don't really understand this part. Could you elaborate why pyxtal project itself get effected by stacklevel=2?

"module" filter suppresses only warnings with identical message and category.

Even if you include "i=" or r"i=.*" in the message argument?

We can simplify the warning message than.I thought I've also added @deprecated for mypy support. Any feedback on a useful message?

I don't really understand this part. Could you elaborate why pyxtal project itself get effected by stacklevel=2?

Quoting from the python doc:

The stacklevel argument can be used by wrapper functions written in Python, like this:

def deprecated_api(message):
    warnings.warn(message, DeprecationWarning, stacklevel=2)

This makes the warning refer to deprecated_api’s caller, rather than to the source of deprecated_api itself (since the latter would defeat the purpose of the warning message).

...

The stacklevel determines where the warning is emitted. If it is 1 (the default), the warning is emitted at the direct caller of the deprecated object; if it is higher, it is emitted further up the stack. Static type checker behavior is not affected by the category and stacklevel arguments.

To my reading we should be emitting the warning when it calls __getitem__ and not when someone else calls the pyxtal api. My concern is that if you call a foo function with high stacklevel the warning would not be emitted (will test it myself later) or it would take the context of the caller way later in the stack defeating the filterwarnings control.

Even if you include "i=" or r"i=.*" in the message argument?

Yes, no matter filterwarnings have message argument, warnings with different messages will be treated as different warnings and get suppressed by filters independently.

python3 -c '   
import warnings                                                         
warnings.filterwarnings("module", category=UserWarning, message=r"i=.*")

for i in range(10):
  warnings.warn(f"i={i}", UserWarning)
for i in range(10):
  warnings.warn(f"i={i}", UserWarning)
'
<string>:6: UserWarning: i=0
<string>:6: UserWarning: i=1
<string>:6: UserWarning: i=2
<string>:6: UserWarning: i=3
<string>:6: UserWarning: i=4
<string>:6: UserWarning: i=5
<string>:6: UserWarning: i=6
<string>:6: UserWarning: i=7
<string>:6: UserWarning: i=8
<string>:6: UserWarning: i=9

My concern is that if you call a foo function with high stacklevel the warning would not be emitted

What do you mean by call a foo function with high stacklevel? I don't think there is a way to change stacklevel from the caller.

FWIW, if stacklevel is larger than the size of the actual stack trace, it will be emitted like this.

sys:1: UserWarning: stacklevel=99

or it would take the context of the caller way later in the stack defeating the filterwarnings control.

I'm not sure what you mean but setting stacklevel does not delay the timing when the warning is emitted. It just refer the stack trace and emit the warning immediately as if it is emitted from the module/line of specified level of stack.

I'm afraid my intension was not clear so let me clarify my goal.

Suppose 1st party and 3rd party library using deprecated methods from 2nd party library like below.

# second.py
def deprecated():
    warnings.warn('bla', DeprecationWarning)

# third.py
def foo():
    second.deprecated()

# first.py
def bar():
    second.deprecated()

def baz():
    third.foo()

My goal is to treat warnings only from first.py as errors and not from third.py. To do that, warnings.warn in deprecated() should have stacklevel=2 and filterwarnings should be like below.

ignore::DeprecationWarning
error::DeprecationWarning::first

Side note, if stacklevel=2 is passed, warning lines will include caller modules' names, not spglib itself. So it will be clear to viewers that the problem is in the caller, not in spglib.

My goal is to treat warnings only from first.py as errors and not from third.py

Yes, but I don't think it's the appropriate way for that. I need to think this a bit and look at some other references. There is a reason why the default is stacklevel=1.

What about using @warnings.deprecated and mypy checking. Isn't that a better tool for that? Let me see how the filterwarnings work with the stacklevel.

Another design in your example is to use with warnings.catch_warnings in baz(), independent of you having the bar() function. Because you are already aware that foo() function is using deprecated api, so you report to third to request a change and on your end you disable the warning in that context and possibly try...catch it so that your users are not affected.

Ok I've done some experimentation and the stacklevel behaves very "interesting", check this gist

• stacklevel=0 -> no warnings
• stacklevel=1 -> warnings on bar['val']
  Weird thing is that foo['val'] does not trigger the warning
• stacklevel=2 -> warnings on call_foo and call_bar
• stacklevel=3 -> warnings on case_1/case_2

Using the warnings.warn manually has the exact same effect, but offset by one stacklevel. At firthst glance that would seem to be resolving the issue, but then, consider moving the call_bar function to another module, move case_1 as well, etc., then the warnings are again not reliably triggered. But this seems to be an issue with the defaults of filterwanings default or my IDE, because when I override the filterwarnings(category=DeprecationWarning), I get more reliable replication.

As for my hunch that stacklevel affects the context for the filterwarnings, that is indeed correct, so the question is which stacklevel is correct to use here.

1. warn(stacklevel=1): We can guarantee that only one warning due to DictInterface is triggered, but depending on the order it can come from either first-party or third-party and it does not show reliably where it comes from
   This is not achievable with warnings.deprecated
2. deprecated(stacklevel=1)/warn(stacklevel=2): The context is from the caller of the deprecated api
3. higher stacklevel: undefined behavior

Overall I think you are right with the approach here and setting the stacklevel=2 for the DictInterface, but we need to check more carefully for the other ones. @ktns if it's ok with you I will re-implement this with warnings.deprecated instead, and change the warning message so that it can be filtered more reliably.

Please let me take time to understand your concerns...

Weird thing is that foo['val'] does not trigger the warning

This is because by default python suppresses warnings with identical message/category/module/line second time or later.

Please coordinate with #538.

@LecrisUT Sorry I didn't see this PR (only searched issues)

I agree with this change, but my suggestion is to go with warnings.deprecated. I plan to work on it, but not sure if @ktns would want to research the issue more on their side.

warnings.deprecated is clean, but it's added in Python 3.13, so I don't think we could use it right now.

Added in version 3.13: See PEP 702.

warnings.deprecated is clean, but it's added in Python 3.13, so I don't think we could use it right now.

Added in version 3.13: See PEP 702.

This is backported in typing-extensions 4.5.0 for mypy support and 4.9.0 for the current version in warnings.deprecated

https://typing-extensions.readthedocs.io/en/latest/#typing_extensions.deprecated

This is backported in typing-extensions 4.5.0 for mypy support and 4.9.0 for the current version in warnings.deprecated

I guess that means we would need an additional dependency for 3.10 <= Python < 3.13 (typing-extensions is not stdlib):

Would that be a very high ask to install an additional dependency just to use typing_extensions.deprecated?

This is backported in typing-extensions 4.5.0 for mypy support and 4.9.0 for the current version in warnings.deprecated

I guess that means we would need an additional dependency for 3.10 <= Python < 3.13 (typing-extensions is not stdlib):

spglib/pyproject.toml

Line 18 in 8a968e4

"typing-extensions; python_version<'3.10'",

Would that be a very high ask to install an additional dependency just to use typing_extensions.deprecated?

Not at all and that is good practice (that is how these packages are meant to be used), we just need to update the dependencies. What I strive for is to make sure at least the LTS distros have all the required dependencies necessary, but so far I only have access to Fedora based ones through EPEL, so those are my targets on deciding if these can be added. Other environments like conda, pypi, spack, homebrew, have more freedom on getting the required version of dependencies so it's not much of a worry there

would also appreciate this PR being merged. didn't read the entire thread so apologies if this is redundant but let me link https://docs.astral.sh/ruff/rules/no-explicit-stacklevel for why passing explicit stacklevel>1 is almost always advisable

@LecrisUT, is it possible to merge this PR and then make another PR to improve it?

link https://docs.astral.sh/ruff/rules/no-explicit-stacklevel for why passing explicit stacklevel>1 is almost always advisable

This is worth looking and link two more detailed explanation on the stacktrace behaviour related to warnings in case they would help make the decision (this is a great and informative channel recommended by Janosh BTW):

• python warnings got way better in 3.12!
• python warnings defaults suck (intermediate) anthony explains

In my opinion though two stacks up should suffice for a deprecated API, see https://docs.python.org/3/library/warnings.html#warnings.warn:

The stacklevel argument can be used by wrapper functions written in Python, like this:

def deprecated_api(message):
warnings.warn(message, DeprecationWarning, stacklevel=2)