D5550: rust-cpython: bindings for MissingAncestors

[kevincox (Kevin Cox)]

kevincox added inline comments.

INLINE COMMENTS

> gracinet wrote in ancestors.rs:170
> Got this one in the wrong direction (sorry about that!) : in this case, this 
> is a conversion from Rust, so we de have proper `size_hint()`
> Still, I'll keep in mind the question for the other direction (in which we 
> are using `collect()` already.

That makes sense. Also if you still want to use iterators without wiring up 
size hints properly you could do something like:

  let v = Vector::with_capacity(hint);
  v.extend(iterators);

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[gracinet (Georges Racinet)]

gracinet marked 2 inline comments as done.
gracinet added inline comments.

INLINE COMMENTS

> gracinet wrote in ancestors.rs:170
> Thanks for the detailed answer.
> 
> Then the problem, in case of iterators coming directly from Python, is that 
> they don't currently implement `size_hint()`, so there's an improvement to be 
> done at this level (many Pythoin iterators do have a `__length_hint()` 
> method, so it shouldn't be a problem to reuse it)
> 
> I have currently some suspicions that the conversions between Rust and Python 
> are the bottleneck in some important cases, so I'll have to measure this 
> things anyway. I won't touch that immediately, but I'll get back to it with 
> real soon.

Got this one in the wrong direction (sorry about that!) : in this case, this is 
a conversion from Rust, so we de have proper `size_hint()`
Still, I'll keep in mind the question for the other direction (in which we are 
using `collect()` already.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[gracinet (Georges Racinet)]

gracinet added inline comments.

INLINE COMMENTS

> kevincox wrote in ancestors.rs:170
> Iterator has a size_hint 
>  
> method which can provide a clue as to the size of the iterator if know. There 
> is also the ExactSizeIterator trait but it is sufficient to say that most 
> simple operations on slice (or Vec) iterators will maintain the size hint and 
> that collecting into a vector will be the most efficient way to construct a 
> vector.
> 
> In theory the performance could be slightly better for the collect approach 
> as you could avoid some bounds checking and incrementing the size each time 
> but in practice I would expect similar performance.
> 
> So the TL;DR is don't worry about collect performance especially for the 
> simple situations. If there are more allocations then necessary then the bug 
> is in the rust `std` crate.

Thanks for the detailed answer.

Then the problem, in case of iterators coming directly from Python, is that 
they don't currently implement `size_hint()`, so there's an improvement to be 
done at this level (many Pythoin iterators do have a `__length_hint()` method, 
so it shouldn't be a problem to reuse it)

I have currently some suspicions that the conversions between Rust and Python 
are the bottleneck in some important cases, so I'll have to measure this things 
anyway. I won't touch that immediately, but I'll get back to it with real soon.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[kevincox (Kevin Cox)]

kevincox added inline comments.

INLINE COMMENTS

> gracinet wrote in ancestors.rs:170
> Hi @kevincox. I have a general question for these, please correct me if I'm 
> making wrong assumptions.
> 
> Given that we know in advance exactly the number of elements required, and 
> that it can be large, doesn't it add overhead to convert everything to 
> collection of iterators? I suppose the `Vec` would have to grow several 
> times, and that's as many calls to `malloc()` inernally. I'm not sure at this 
> point it would be very costly, but in this case where we consume the `Vec` 
> immediately, is there a reason to believe that going the `collect()` way 
> could have their own performance benefits?
> 
> Note: this code landed, but I'm about to submit some related refactors, and 
> in this specific case, it's going to be replace I hope very soon with a call 
> to `PySet`, but I'm asking in general.
> 
> Cheers,

Iterator has a size_hint 
 
method which can provide a clue as to the size of the iterator if know. There 
is also the ExactSizeIterator trait but it is sufficient to say that most 
simple operations on slice (or Vec) iterators will maintain the size hint and 
that collecting into a vector will be the most efficient way to construct a 
vector.

In theory the performance could be slightly better for the collect approach as 
you could avoid some bounds checking and incrementing the size each time but in 
practice I would expect similar performance.

So the TL;DR is don't worry about collect performance especially for the simple 
situations. If there are more allocations then necessary then the bug is in the 
rust `std` crate.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[gracinet (Georges Racinet)]

gracinet added inline comments.

INLINE COMMENTS

> kevincox wrote in ancestors.rs:170
> This could be a `.collect()` call. Something like:
> 
>   let bases_vec: Vec = bases_set.into_iter()
>   .map(|rev| rev.to_py_object(py).into_object())
>   .collect();

Hi @kevincox. I have a general question for these, please correct me if I'm 
making wrong assumptions.

Given that we know in advance exactly the number of elements required, and that 
it can be large, doesn't it add overhead to convert everything to collection of 
iterators? I suppose the `Vec` would have to grow several times, and that's as 
many calls to `malloc()` inernally. I'm not sure at this point it would be very 
costly, but in this case where we consume the `Vec` immediately, is there a 
reason to believe that going the `collect()` way could have their own 
performance benefits?

Note: this code landed, but I'm about to submit some related refactors, and in 
this specific case, it's going to be replace I hope very soon with a call to 
`PySet`, but I'm asking in general.

Cheers,

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[kevincox (Kevin Cox)]

kevincox added inline comments.

INLINE COMMENTS

> ancestors.rs:170
> +bases_vec.push(rev.to_py_object(py).into_object());
> +}
> +Ok(PyTuple::new(py, bases_vec.as_slice()))

This could be a `.collect()` call. Something like:

  let bases_vec: Vec = bases_set.into_iter()
  .map(|rev| rev.to_py_object(py).into_object())
  .collect();

> ancestors.rs:193
> +remaining_pyint_vec.push(rev.to_py_object(py).into_object());
> +}
> +let remaining_pylist = PyList::new(py, 
> remaining_pyint_vec.as_slice());

This could also be a `collect()`.

> ancestors.rs:204
> +Err(e) => {
> +return Err(GraphError::pynew(py, e));
> +}

This match can become a `.map_error(|e| GraphError::pynew(py, e))?`

> ancestors.rs:212
> +missing_pyint_vec.push(rev.to_py_object(py).into_object());
> +}
> +Ok(PyList::new(py, missing_pyint_vec.as_slice()))

This can also be a `.collect()`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D5550: rust-cpython: bindings for MissingAncestors

[Georges Racinet]

On 1/11/19 3:10 PM, yuja (Yuya Nishihara) wrote:
> yuja added a comment.
>
>
>   Queued up to this patch, thanks.
Great, thanks !   
>   
>   > +def __new__(_cls, index: PyObject, bases: PyObject) -> 
> PyResult {
>   >  +let bases_vec: Vec = rev_pyiter_collect(py, )?;
>   >  +let inner = CoreMissing::new(Index::new(py, index)?, bases_vec);
>   
>   We might want to directly build `HashSet` here if that matters.
>   
>   > +def missingancestors(, revs: PyObject) -> PyResult {
>   >  +let mut inner = self.inner(py).borrow_mut();
>   >  +let revs_vec: Vec = rev_pyiter_collect(py, )?;
>   >  +let missing_vec = match inner.missing_ancestors(revs_vec) {
>   >  +Ok(missing) => missing,
>   >  +Err(e) => {
>   >  +return Err(GraphError::pynew(py, e));
>   >  +}
>   >  +};
>   
>   
>   
>   > +// convert as Python list
>   >  +let mut missing_pyint_vec: Vec = Vec::with_capacity(
>   >  +missing_vec.len());
>   >  +for rev in missing_vec {
>   >  +missing_pyint_vec.push(rev.to_py_object(py).into_object());
>   >  +}
>   >  +Ok(PyList::new(py, missing_pyint_vec.as_slice()))
>   
>   Maybe this can be extracted to a helper function so that we can `.map()`
>   the result.
>
> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D5550
>
> To: gracinet, #hg-reviewers
> Cc: yuja, durin42, kevincox, mercurial-devel
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

-- 
Georges Racinet
https://octobus.net
GPG: BF5456F4DC625443849B6E58EE20CA44EF691D39, sur serveurs publics




signature.asc
Description: OpenPGP digital signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[gracinet (Georges Racinet)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHG006c9ce486fa: rust-cpython: bindings for MissingAncestors 
(authored by gracinet, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5550?vs=13128=13174

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

AFFECTED FILES
  rust/hg-cpython/src/ancestors.rs
  tests/test-rust-ancestor.py

CHANGE DETAILS

diff --git a/tests/test-rust-ancestor.py b/tests/test-rust-ancestor.py
--- a/tests/test-rust-ancestor.py
+++ b/tests/test-rust-ancestor.py
@@ -11,7 +11,8 @@
 # this would fail already without appropriate ancestor.__package__
 from mercurial.rustext.ancestor import (
 AncestorsIterator,
-LazyAncestors
+LazyAncestors,
+MissingAncestors,
 )
 
 try:
@@ -105,6 +106,22 @@
 # let's check bool for an empty one
 self.assertFalse(LazyAncestors(idx, [0], 0, False))
 
+def testmissingancestors(self):
+idx = self.parseindex()
+missanc = MissingAncestors(idx, [1])
+self.assertTrue(missanc.hasbases())
+self.assertEqual(missanc.missingancestors([3]), [2, 3])
+missanc.addbases({2})
+self.assertEqual(set(missanc.bases()), {1, 2})
+self.assertEqual(missanc.missingancestors([3]), [3])
+
+def testmissingancestorsremove(self):
+idx = self.parseindex()
+missanc = MissingAncestors(idx, [1])
+revs = {0, 1, 2, 3}
+missanc.removeancestorsfrom(revs)
+self.assertEqual(revs, {2, 3})
+
 def testrefcount(self):
 idx = self.parseindex()
 start_count = sys.getrefcount(idx)
diff --git a/rust/hg-cpython/src/ancestors.rs b/rust/hg-cpython/src/ancestors.rs
--- a/rust/hg-cpython/src/ancestors.rs
+++ b/rust/hg-cpython/src/ancestors.rs
@@ -15,22 +15,39 @@
 //!   The only difference is that it is instantiated with a C `parsers.index`
 //!   instance instead of a parents function.
 //!
+//! - [`MissingAncestors`] is the Rust implementation of
+//!   `mercurial.ancestor.incrementalmissingancestors`.
+//!
+//!   API differences:
+//!+ it is instantiated with a C `parsers.index`
+//!  instance instead of a parents function.
+//!+ `MissingAncestors.bases` is a method returning a tuple instead of
+//!  a set-valued attribute. We could return a Python set easily if our
+//!  [PySet PR](https://github.com/dgrunwald/rust-cpython/pull/165)
+//!  is accepted.
+//!
 //! - [`AncestorsIterator`] is the Rust counterpart of the
 //!   `ancestor._lazyancestorsiter` Python generator.
 //!   From Python, instances of this should be mainly obtained by calling
 //!   `iter()` on a [`LazyAncestors`] instance.
 //!
 //! [`LazyAncestors`]: struct.LazyAncestors.html
+//! [`MissingAncestors`]: struct.MissingAncestors.html
 //! [`AncestorsIterator`]: struct.AncestorsIterator.html
 use cindex::Index;
 use cpython::{
-ObjectProtocol, PyClone, PyDict, PyModule, PyObject, PyResult, Python,
+ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject,
+PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
 use exceptions::GraphError;
 use hg::Revision;
-use hg::{AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy};
+use hg::{
+AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy,
+MissingAncestors as CoreMissing,
+};
 use std::cell::RefCell;
 use std::iter::FromIterator;
+use std::collections::HashSet;
 
 /// Utility function to convert a Python iterable into various collections
 ///
@@ -119,7 +136,85 @@
 
 });
 
-/// Create the module, with `__package__` given from parent
+py_class!(pub class MissingAncestors |py| {
+data inner: RefCell>>;
+
+def __new__(_cls, index: PyObject, bases: PyObject) -> 
PyResult {
+let bases_vec: Vec = rev_pyiter_collect(py, )?;
+let inner = CoreMissing::new(Index::new(py, index)?, bases_vec);
+MissingAncestors::create_instance(py, RefCell::new(Box::new(inner)))
+}
+
+def hasbases() -> PyResult {
+Ok(self.inner(py).borrow().has_bases())
+}
+
+def addbases(, bases: PyObject) -> PyResult {
+let mut inner = self.inner(py).borrow_mut();
+let bases_vec: Vec = rev_pyiter_collect(py, )?;
+inner.add_bases(bases_vec);
+// cpython doc has examples with PyResult<()> but this gives me
+//   the trait `cpython::ToPyObject` is not implemented for `()`
+// so let's return an explicit None
+Ok(py.None())
+}
+
+def bases() -> PyResult {
+let inner = self.inner(py).borrow();
+let bases_set = inner.get_bases();
+// convert as Python tuple TODO how to return a proper Python set?
+let mut bases_vec: Vec = Vec::with_capacity(
+bases_set.len());
+for rev in bases_set {
+bases_vec.push(rev.to_py_object(py).into_object());
+}
+Ok(PyTuple::new(py, bases_vec.as_slice()))
+}
+
+def 

D5550: rust-cpython: bindings for MissingAncestors

[yuja (Yuya Nishihara)]

yuja added a comment.


  Queued up to this patch, thanks.
  
  > +def __new__(_cls, index: PyObject, bases: PyObject) -> 
PyResult {
  >  +let bases_vec: Vec = rev_pyiter_collect(py, )?;
  >  +let inner = CoreMissing::new(Index::new(py, index)?, bases_vec);
  
  We might want to directly build `HashSet` here if that matters.
  
  > +def missingancestors(, revs: PyObject) -> PyResult {
  >  +let mut inner = self.inner(py).borrow_mut();
  >  +let revs_vec: Vec = rev_pyiter_collect(py, )?;
  >  +let missing_vec = match inner.missing_ancestors(revs_vec) {
  >  +Ok(missing) => missing,
  >  +Err(e) => {
  >  +return Err(GraphError::pynew(py, e));
  >  +}
  >  +};
  
  
  
  > +// convert as Python list
  >  +let mut missing_pyint_vec: Vec = Vec::with_capacity(
  >  +missing_vec.len());
  >  +for rev in missing_vec {
  >  +missing_pyint_vec.push(rev.to_py_object(py).into_object());
  >  +}
  >  +Ok(PyList::new(py, missing_pyint_vec.as_slice()))
  
  Maybe this can be extracted to a helper function so that we can `.map()`
  the result.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

To: gracinet, #hg-reviewers
Cc: yuja, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D5550: rust-cpython: bindings for MissingAncestors

[Yuya Nishihara]

Queued up to this patch, thanks.

> +def __new__(_cls, index: PyObject, bases: PyObject) -> 
> PyResult {
> +let bases_vec: Vec = rev_pyiter_collect(py, )?;
> +let inner = CoreMissing::new(Index::new(py, index)?, bases_vec);

We might want to directly build `HashSet` here if that matters.

> +def missingancestors(, revs: PyObject) -> PyResult {
> +let mut inner = self.inner(py).borrow_mut();
> +let revs_vec: Vec = rev_pyiter_collect(py, )?;
> +let missing_vec = match inner.missing_ancestors(revs_vec) {
> +Ok(missing) => missing,
> +Err(e) => {
> +return Err(GraphError::pynew(py, e));
> +}
> +};

> +// convert as Python list
> +let mut missing_pyint_vec: Vec = Vec::with_capacity(
> +missing_vec.len());
> +for rev in missing_vec {
> +missing_pyint_vec.push(rev.to_py_object(py).into_object());
> +}
> +Ok(PyList::new(py, missing_pyint_vec.as_slice()))

Maybe this can be extracted to a helper function so that we can `.map()`
the result.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D5550: rust-cpython: bindings for MissingAncestors

[gracinet (Georges Racinet)]

gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The exposition is rather straightforward, except for the
  remove_ancestors_from() method, which forces us to an inefficient
  conversion between Python sets and Rust HashSets.
  
  Two alternatives are proposed in comments:
  
  - changing the inner API to "emit" the revision numbers to discard this would 
be a substantial change, and it would be better only in the cases where there 
are more to retain than to discard
  - mutating the Python set directly: this would force us to define an abstract 
`RevisionSet` trait, and implement it both for plain `HashSet` and for a struct 
enclosing a Python set with the GIL marker `Python<'p>`, also a non trivial 
effort.
  
  The main (and seemingly only) caller of this method being
  `mercurial.setdiscovery`, which is currently undergoing serious refactoring,
  it's not clear whether these improvements would be worth the effort right now,
  so we're leaving it as-is.
  
  Also, in `get_bases()` (will also be used by `setdiscovery`), we'd prefer
  to build a Python set directly, but we resort to returning a tuple, waiting
  to hear back from our PR onto rust-cpython about that

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5550

AFFECTED FILES
  rust/hg-cpython/src/ancestors.rs
  tests/test-rust-ancestor.py

CHANGE DETAILS

diff --git a/tests/test-rust-ancestor.py b/tests/test-rust-ancestor.py
--- a/tests/test-rust-ancestor.py
+++ b/tests/test-rust-ancestor.py
@@ -11,7 +11,8 @@
 # this would fail already without appropriate ancestor.__package__
 from mercurial.rustext.ancestor import (
 AncestorsIterator,
-LazyAncestors
+LazyAncestors,
+MissingAncestors,
 )
 
 try:
@@ -105,6 +106,22 @@
 # let's check bool for an empty one
 self.assertFalse(LazyAncestors(idx, [0], 0, False))
 
+def testmissingancestors(self):
+idx = self.parseindex()
+missanc = MissingAncestors(idx, [1])
+self.assertTrue(missanc.hasbases())
+self.assertEqual(missanc.missingancestors([3]), [2, 3])
+missanc.addbases({2})
+self.assertEqual(set(missanc.bases()), {1, 2})
+self.assertEqual(missanc.missingancestors([3]), [3])
+
+def testmissingancestorsremove(self):
+idx = self.parseindex()
+missanc = MissingAncestors(idx, [1])
+revs = {0, 1, 2, 3}
+missanc.removeancestorsfrom(revs)
+self.assertEqual(revs, {2, 3})
+
 def testrefcount(self):
 idx = self.parseindex()
 start_count = sys.getrefcount(idx)
diff --git a/rust/hg-cpython/src/ancestors.rs b/rust/hg-cpython/src/ancestors.rs
--- a/rust/hg-cpython/src/ancestors.rs
+++ b/rust/hg-cpython/src/ancestors.rs
@@ -15,22 +15,39 @@
 //!   The only difference is that it is instantiated with a C `parsers.index`
 //!   instance instead of a parents function.
 //!
+//! - [`MissingAncestors`] is the Rust implementation of
+//!   `mercurial.ancestor.incrementalmissingancestors`.
+//!
+//!   API differences:
+//!+ it is instantiated with a C `parsers.index`
+//!  instance instead of a parents function.
+//!+ `MissingAncestors.bases` is a method returning a tuple instead of
+//!  a set-valued attribute. We could return a Python set easily if our
+//!  [PySet PR](https://github.com/dgrunwald/rust-cpython/pull/165)
+//!  is accepted.
+//!
 //! - [`AncestorsIterator`] is the Rust counterpart of the
 //!   `ancestor._lazyancestorsiter` Python generator.
 //!   From Python, instances of this should be mainly obtained by calling
 //!   `iter()` on a [`LazyAncestors`] instance.
 //!
 //! [`LazyAncestors`]: struct.LazyAncestors.html
+//! [`MissingAncestors`]: struct.MissingAncestors.html
 //! [`AncestorsIterator`]: struct.AncestorsIterator.html
 use cindex::Index;
 use cpython::{
-ObjectProtocol, PyClone, PyDict, PyModule, PyObject, PyResult, Python,
+ObjectProtocol, PyClone, PyDict, PyList, PyModule, PyObject,
+PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
 use exceptions::GraphError;
 use hg::Revision;
-use hg::{AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy};
+use hg::{
+AncestorsIterator as CoreIterator, LazyAncestors as CoreLazy,
+MissingAncestors as CoreMissing,
+};
 use std::cell::RefCell;
 use std::iter::FromIterator;
+use std::collections::HashSet;
 
 /// Utility function to convert a Python iterable into various collections
 ///
@@ -119,7 +136,85 @@
 
 });
 
-/// Create the module, with `__package__` given from parent
+py_class!(pub class MissingAncestors |py| {
+data inner: RefCell>>;
+
+def __new__(_cls, index: PyObject, bases: PyObject) -> 
PyResult {
+let bases_vec: Vec = rev_pyiter_collect(py, )?;
+let inner = CoreMissing::new(Index::new(py, index)?, bases_vec);
+MissingAncestors::create_instance(py,