Skip to content
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

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

neutrinoceros
Copy link
Member

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.

@neutrinoceros neutrinoceros added the enhancement Making something better label Mar 2, 2022
@neutrinoceros neutrinoceros marked this pull request as ready for review March 2, 2022 23:32
@neutrinoceros neutrinoceros added this to the 4.0.3 milestone Mar 2, 2022
cphyc
cphyc previously approved these changes Mar 3, 2022
Copy link
Member

@cphyc cphyc left a 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)

@neutrinoceros
Copy link
Member Author

@cphyc I suppose that handling this kind of problems would be easier if #3626 gets accepted.
Note that this behaviour, desired or not, is consistent with that of the main branch where you'd get toml.decoder.TomlDecodeError: Found invalid character in key name: 't'. Try quoting the key name. (line 1 column 3 char 2), so while I agree we should discuss it, it's not a breaking change so I think it's safe to backport to 4.0.x

setup.cfg Outdated
tqdm>=3.4.0
unyt>=2.8.0
tomli>=1.2.3;python_version < '3.11'
Copy link
Member Author

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

@neutrinoceros
Copy link
Member Author

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
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

You got it.

@cphyc cphyc enabled auto-merge March 3, 2022 11:18
@cphyc cphyc merged commit 564450a into yt-project:main Mar 3, 2022
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 3, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout yt-4.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 564450aa2b0efe5195811b11c533561f4926adca
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3831: MNT: migrate from toml to tomli + tomli_w'
  1. Push to a named branch:
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3831-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3831 on branch yt-4.0.x (MNT: migrate from toml to tomli + tomli_w)"

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 Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@neutrinoceros neutrinoceros deleted the migrate2tomli branch March 3, 2022 12:42
neutrinoceros pushed a commit to neutrinoceros/yt that referenced this pull request Mar 3, 2022
matthewturk added a commit that referenced this pull request Mar 4, 2022
Manual backport #3831 to yt-4.0.x (MNT: migrate from toml to tomli + tomli_w)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert to using tomli
3 participants