-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MNT: migrate from toml to tomli + tomli_w #3831
Conversation
1e5d6df
to
ec3845c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and this works like a charm!
On a side note - I realised that if the config is malformed yt cannot load properly. Note that is not new with this PR (as of ff5e808 a toml.decoder.TomlDecodeError
is raised instead). Is this something we want? For example the following
echo "! this is invalid" > yt.toml
python -c "import yt"
fails with the backtrace
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/XXX/Documents/prog/yt/yt/__init__.py", line 13, in <module>
import yt.utilities.physical_constants as physical_constants
File "/home/XXX/Documents/prog/yt/yt/utilities/physical_constants.py", line 3, in <module>
from yt.units.yt_array import YTQuantity
File "/home/XXX/Documents/prog/yt/yt/units/yt_array.py", line 3, in <module>
from yt.funcs import array_like_field # NOQA: F401
File "/home/XXX/Documents/prog/yt/yt/funcs.py", line 33, in <module>
from yt.utilities.logger import ytLogger as mylog
File "/home/XXX/Documents/prog/yt/yt/utilities/logger.py", line 4, in <module>
from yt.config import ytcfg
File "/home/XXX/Documents/prog/yt/yt/config.py", line 215, in <module>
ytcfg.read(_local_config_file)
File "/home/XXX/Documents/prog/yt/yt/config.py", line 154, in read
data = tomllib.load(fh)
File "/home/XXX/anaconda3/envs/py39/lib/python3.9/site-packages/tomli/_parser.py", line 66, in load
return loads(s, parse_float=parse_float)
File "/home/XXX/anaconda3/envs/py39/lib/python3.9/site-packages/tomli/_parser.py", line 116, in loads
raise suffixed_err(src, pos, "Invalid statement")
tomli.TOMLDecodeError: Invalid statement (at line 1, column 1)
@cphyc I suppose that handling this kind of problems would be easier if #3626 gets accepted. |
setup.cfg
Outdated
tqdm>=3.4.0 | ||
unyt>=2.8.0 | ||
tomli>=1.2.3;python_version < '3.11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the record, I picked the most recent versions of each package that had support for Python 3.6 as minimal, to make backporting to yt 4.0.x easier
ec3845c
to
9446e89
Compare
Sorry for pushing again, I changed my mind about the 3.11 branch. I think it's preferable to keep one robust and testable solution for now, and make tomli a dependency for Python <= 3.10 only when Python 3.11 becomes testable to us (probably sometimes this summer). |
@@ -1,7 +1,9 @@ | |||
import os | |||
import warnings | |||
|
|||
import toml | |||
# TODO: import tomllib from the standard library instead in Python >= 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely going to break some developer installations where folks do an in-place without dependency checks, so we should send a note to the mailing list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You got it.
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
MNT: migrate from toml to tomli + tomli_w
Manual backport #3831 to yt-4.0.x (MNT: migrate from toml to tomli + tomli_w)
PR Summary
Fix #3830. Possibly not exactly the correct approach right now (I'm adding a conditional branch for Python 3.11 before it even has the feature implemented), but I'd like to see if this naive approach survives CI.