CodSpeed Performance Report

Merging #15532 will not alter performance

_{Comparing agriyakhetarpal:deprecation-warnings-as-errors-pt2 (fe0080c) with main (9a146a8)}

Summary

✅ 23 untouched
⏩ 21 skipped¹

@agriyakhetarpal,

I looked around on this and learned that it's not actually necessary to add this our CondaJSONEncoder because this is already being accounted for in frozendict itself.

Please see the following links for more information:

• https://github.com/Marco-Sulla/python-frozendict/releases/tag/v2.3.6
• Lost JSON serialising ability with frozendict==2.2.0 Marco-Sulla/python-frozendict#47

Conda currently has a requirement of frozendict>=2.4.2, so this shouldn't be an issue. When we finally remove the deprecated code paths, everything should JustWork™️ ✨ .

@jezdez,

Do you have any feedback on my comment? Am I correct in thinking that this pull request isn't necessary?

Oh huh, that comment didn't load in the tab I had this issue open before posting my comment. This changes things!

Looking at the change upstream, this monkeypatch seems like a pretty bad idea, Marco-Sulla/python-frozendict@v2.3.5...v2.3.6#diff-f9ec75e660 and we added the pending deprecation note in #14709 specifically to stop monkeypatching locally as well. I would say this change here is technically correct, but you're right that the monkeypatching in https://github.com/Marco-Sulla/python-frozendict/blob/master/src/frozendict/monkeypatch.py seems to cover most cases. I wonder if there is any risk involved with the change in #14709 that we hadn't thought about. This is why monkeypatching is never a good API strategy!

This makes me want to stop relying on frozendict btw.

I wonder if https://peps.python.org/pep-0814/ goes anywhere and if there are backports? What about https://adamj.eu/tech/2022/01/05/how-to-make-immutable-dict-in-python/ should we simply roll it ourselves?

This makes me want to stop relying on frozendict btw.

My thoughts exactly! 😬

I wonder if https://peps.python.org/pep-0814/ goes anywhere and if there are backports? What about https://adamj.eu/tech/2022/01/05/how-to-make-immutable-dict-in-python/ should we simply roll it ourselves?

Maybe we should just create a separate issue for this work and not merge this pull request?

@kenodegard, maybe you have some thoughts on this. Couldn't help but notice your name as I was skimming through the release notes in frozendict.

Thanks for the conversation, y'all! For now, I'll remove this change from #15525 and replace it with a deprecated_call there, and I'll mark this pull request as a draft. I don't yet see a backport for PEP 814 work, so maybe having our own frozendict in auxlib is the best idea.

This PR has been superseded by the change I described in #15525 (comment). Thanks, everyone!

I'm reopening this PR as per @beckermr's suggestion in #15525 (comment). The support that frozendict provides for custom encoders like ours is a bit finicky – mainly because it requires us to mess with the import order and ensure that frozendict is imported before json.

Hi @beckermr,

Thanks for chiming in here and looking into this too. I read your feedback over here and think it's reasonable to include this pull request. Inspecting the type here doesn't have a huge cost and we add additional tests to ensure this expect behavior works.

Because of those reasons, I'll go ahead merge this pull request. But, with that being said, I think it's worth it to research what we could do instead of relying on a dependency that monkey patches the standard library. I think we can all agree that that's not a good idea, so I will create an issue specifically for that work.