Fix a regression introduced in gh-727 and gh-723, caused by a misunderstanding of how __new__ works #733
Conversation
| def test_edge_identity(dut): | ||
| """ | ||
| Test that Edge triggers returns the same object each time | ||
| """ |
There was a problem hiding this comment.
This test passed before this patch too, but I figured it was worth adding anyway
|
Reiterating the goals of the original patches, I wanted to:
There are some compromises we could make here;
|
|
@imphil: Would be great to hear your opinions on the above approaches. |
|
@eric-wieser thanks for following up on the issue so quickly. I hope to find time tomorrow during a flight to have a closer look. |
|
OK, I took a closer look and I'm not sure I fully understand all the side effects and requirements, but wouldn't the following work as well? diff --git a/cocotb/triggers.py b/cocotb/triggers.py
index a489294..5472b17 100644
--- a/cocotb/triggers.py
+++ b/cocotb/triggers.py
@@ -230,11 +230,15 @@ class _EdgeBase(GPITrigger):
except KeyError:
instance = super(_EdgeBase, cls).__new__(cls)
cls._instances[key] = instance
+ instance._initialized = False
return instance
def __init__(self, signal):
+ if self._initialized:
+ return
super(_EdgeBase, self).__init__()
self.signal = signal
+ self._initialized = True
def prime(self, callback):
"""Register notification of a value change via a callback"""Isn't the construct we're looking for a simple singleton? It seems too obvious so I probably missed something? |
|
We're looking for a parameterized Singleton, which is essentially the same thing, yes. Your approach would work, but I think it would leave most readers questioning "how could this ever happen", since having init called twice is very unusual. I think I'm leaning towards the approach in my second patch - python will have a built-in |
|
I'm back from my winter vacation, so let's get back to this one. I looked through the implementations in this PR #733 and in #734. I prefer the option presented in this PR (#733) as it is easier understandable to me. The approach in #734 is pretty generic, which makes it hard to understand what practical purpose all this "meta base class stuff" has. The approach in the PR here has a clearly defined and documented purpose, and even reduces code duplication by moving the list of instances into the base. To make it even more obvious what is going on I'd suggest to add a bit of documentation that we're creating a (parametrized) singleton. This clearly defines for probably most readers what's going on. |
|
I vote #733 too. |
Unfortunately most of it is a work around for:
Once the I'll take another look at this in the next few days, adding some more docs. |
yeah, I'm looking forward to that as well. Shouldn't be far away, though. |
|
I'm really not keen on dependencies in the core code. |
eric-wieser
force-pushed
the
trigger-metaclass
branch
from
214ecad
to
8b14098
Compare
|
Updated with a little more documentation, and better naming. Updated the competitor PR too, but I think with the improved naming, this one now wins. I took the liberty of vendoring |
eric-wieser
force-pushed
the
trigger-metaclass
branch
from
ad3ca53
to
b08b03f
Compare
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
B = SomeClass(1)
```
We need to override class-construction without allowing `__init__` to run a second time.
One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.
I'm not convinced this is the best approach, but it does fix the problem.
eric-wieser
force-pushed
the
trigger-metaclass
branch
2 times, most recently
from
16c3e22
to
1ebf47a
Compare
|
Finally passing. I rolled back my attempt to make I'll take another look at that once this goes in, assuming it looks ready to you. |
|
Increasingly concerned with all of this. It's started losing sight of what was originally the issue and adding a lot of complexity. Right now I'm more in favour of backing out the original regression and starting afresh. Seems that people want the package work released and this is now the blocker as we have a regression. I'd like @chiggs to comment. |
|
In the interest of unlocking a release, I'm happy to implement the alternative above I label "Reject 2 and 3, go back to something more similar to the original" (edit: Done in gh-742). I think this patch is still worth considering, but I have no qualms with merging the simpler fix first, and leaving this on the back burner |
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
…by a misunderstanding of how __new__ works
In those patches, I declared classes that were roughly
```python
class SomeClass:
def __new__(cls, arg):
try:
return existing[arg]
except KeyError:
return super(SomeClass, cls).__new__(cls, arg)
def __init__(self, arg):
self.arg = arg
self.state = 0
```
This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A
# A.__init__(1)
B = SomeClass(1)
# SomeClass.__new__(SomeClass, 1) -> A # reusing the existing instance
# A.__init__(1) # uh oh, we reset A.state
```
For the sake of simplicity, this commit undoes the merger between the factory functions and the classes themselves, while keeping the weakref behavior.
In future, it would be good to explore the more complex approach taken in cocotbgh-733, which ensures that `isinstance(Join(), Join)`, which this patch does not.
Since this property was not true before cocotbgh-723 and cocotbgh-727 anyway, we can afford to revert that improvement in order to get a release out the door.
|
I must say I like the implementation proposed in this PR. Singletons in Python are not overly nice to implement, and the implementation we have here makes sense to me: it's understandable and not overly meta'y. If no-one objects I'll merge this one by Friday. |
|
Apologies for the tardy response on this. I like where we’ve ended up here, the final diff is more elegant and understandable than the alternatives so this gets my vote. @eric-wieser thanks for working through possible alternatives as well, it’s very useful to see the goals and reasoning and see how this has evolved with discussion. |
|
If @chiggs is happy I'm happy as well! Merging this to enable wider testing. |
Fix a regression introduced in cocotbgh-727 and cocotbgh-723, caused by a misunderstanding of how __new__ works
Alternative: #734
In those patches (gh-727, gh-723), I declared classes that were roughly
This approach has a fatal flaw (gh-729), with function calls shown in the following code:
We need to override class-construction without allowing
__init__to run a second time.One option would be to just remove
__init__entirely, and move the contents into__new__.The other option, which I take in this patch, is to introduce a metaclass overriding the
__call__operator on class types.I'm not convinced this is the best approach, and is possibly over-engineered - but it does fix the problem.