Fixes several issues found by the new Grapher module work #105
Conversation
…ign escaped when get_value() is called with the sub_vars flag
schuylermartin45
changed the title
|
I put this PR up as a draft to see if the issue I've been having with After more digging, it looks like pickling the The other instances of |
schuylermartin45
changed the title
|
PR is ready to review, this significantly improves our ability to construct comprehensive |
…r into smartin_fix_sub_vars_crash
conda_recipe_manager/types.py
Outdated
| """ | ||
| A single sentinel class to be used in this project, as an alternative to `None` when `None` cannot be used. | ||
| It is defined in a way such that SentinelType instances survive pickling and allocations in different memory | ||
| spaces. | ||
| """ | ||
|
|
||
| def __eq__(self, o: object) -> bool: | ||
| """ | ||
| Provides a pickle/thread-safe way to compare objects to the Sentinel Type. All sentinels are equal by simply | ||
| being sentinel objects. | ||
| :param o: The object to compare against. | ||
| :returns: True if the object is a SentinelType object. | ||
| """ | ||
| # There is an implicit `isinstance(self, SentinelType) and ...` to this logic | ||
| return isinstance(o, SentinelType) |
There was a problem hiding this comment.
I'd suggest reusing some of the sentinel implementations that exist out there, unittest.mock has one, there is https://github.com/Infinidat/sentinels or https://github.com/eddieantonio/sentinel and probably many more.
I don't think your implementation covers the full list of qualities of a sentinel value (e.g. _sentinel isn't able to be compared with is), even though it might fit this particular PR's needs. Nothing wrong with sentinels generally, of course, they are a great pattern.
There was a problem hiding this comment.
I've struggled to find a standard approach here. I would prefer to use something from the standard library, but the mocking version looks specifically designed for testing. I don't think I want unit test helpers in production code.
The other two libraries listed look like they haven't had a release in a few years and I'm unsure what level of support we can expect.
is is not used in any of the sentinel checks that currently exist. For our purposes, the current solution may be good enough for now. We need some kind of sentinel for the RecipeParser, there are a number of places I need to discern the difference between None/Null and "another default of northing".
There was a problem hiding this comment.
It'd be nice if this was accepted: https://peps.python.org/pep-0661/
There was a problem hiding this comment.
It'd be nice if this was accepted: peps.python.org/pep-0661
True!
FWIW, I was suggesting to simply copy over the code, not rely on it as a dependency. They are 3-clause BSD and MIT licensed, so are pretty simple to include, as long as the license statement is kept.
There was a problem hiding this comment.
That said, it was really just a suggestion, not a suggestion wink wink :) I'll have more time to finish the review tomorrow!
conda_recipe_manager/types.py
Outdated
| :returns: True if the object is a SentinelType object. | ||
| """ | ||
| # There is an implicit `isinstance(self, SentinelType) and ...` to this logic | ||
| return isinstance(o, SentinelType) |
There was a problem hiding this comment.
class X:
def __new__(cls):
global singleton
try:
return singleton
except NameError:
singleton = super().__new__(cls)
return singleton
>>> x = X()
>>> x2 = X()
>>> x is x2
True
>>> import pickle
>>> x3 = pickle.loads(pickle.dumps(x2))
>>> x3 is x
TrueThere was a problem hiding this comment.
I think this is in-line with the libraries that @jezdez suggested. But it's also pretty simple, which I like. You've also reminded me that I forgot to add a unit test to pickle this type directly. Thanks!
There was a problem hiding this comment.
Let me take a stab at implementing this version.
There was a problem hiding this comment.
Had to make a few modifications to make the static analyzer and linter happy, but it appears to work!
…r into smartin_fix_sub_vars_crash
…r into smartin_fix_sub_vars_crash
…)" This reverts commit 26eab68.
This PR introduces a few things:
DependencyVariabletype to track dependencies we can't immediately resolve withMatchSpec. This class shares thenameproperty withMatchSpecto ensure some interoperability between the two types.This came out of an issue with some variadic dependencies like
{{ compiler("c") }}. When constructed withMatchSpec, such dependency strings became extremely corrupted.get_value()is called withsub_vars = True.This is done for a few reasons:
SentinelTypeis nowpickle-able so that theRecipeParserclasses can be safely used with themultiprocessinglibrary, without risking data corruption. A new regression test has been added to attempt to catch future pickling issues.