Move MemoryAllocator to PyPI?

[jonatha...@googlemail.com]

Dear all, I have two little projects that I want to put onto PyPI soon. Both of them use MemoryAllocator, because I find it very nice to just allocate memory and let pythons garbage collector take care of it. MemoryAllocator only depends on cysignals, so it would make sense to make it pip installable by itself (or as part of cysignals).

Any opinions on this?

Of course it takes almost no effort just rewrite MemoryAllocator. However, it also makes sense to avoid code duplications.

[Jonathan Kliem]

Or maybe once the modularization has processed (https://trac.sagemath.org/ticket/29705) this really boils down to a tiny ticket?

--
You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/6u1VuXJE7wE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/27836ec2-c5ce-412d-95e5-73363be65c71n%40googlegroups.com.

[Matthias Koeppe]

If it's closely tied to cysignals, perhaps it would be a good idea adding it to this project. I think cysignals needs new contributors in any case.

[Vincent Delecroix]

Why make it part of cysignals? The purpose of the memory allocator
is quite distinct from cysignals. Merging them look artificial to me.

However, +1 to make it available as a Python library.

[Matthias Koeppe]

On Friday, February 12, 2021 at 2:35:45 PM UTC-8 vdelecroix wrote:

Why make it part of cysignals? The purpose of the memory allocator
is quite distinct from cysignals. Merging them look artificial to me.

Really? MemoryAllocator is just a wrapper class around cysignals.memory 

 

[E. Madison Bray]

Was there ever any action on this? MemoryAllocator does seem like a
useful thing to have as an independent package. It's basically a more
sophisticated version of the SomeMemory recipe given in the Cython
docs: https://cython.readthedocs.io/en/latest/src/tutorial/memory_allocation.html

I agree that it's distinct from cysignals. It doesn't necessarily
have to depend on cysignals, and could have a hook interface to allow
different memory allocation functions. It just happens to use the
ones from cysignals since Sage already uses cysignals, and its memory
functions have the advantage of being interrupt-safe.

I would continue using cysignals by default for an initial
implementation, but if anyone had a need for it, it would be possible
to make this work with cysignals as an optional dependency as well.

[Jonathan Kliem]

What I'm struggling with is the following:

How do I cimport from different sources (runtime dependent probably
won't work, but compile time dependent would already be nice). At the
moment my package just pulls in cysignals during pip install and if this
does not work it replaces those functions by standard allocation functions.

Is there a better solution to do this?

[E. Madison Bray]

On Wed, Mar 10, 2021 at 3:51 PM 'Jonathan Kliem' via sage-devel
<sage-...@googlegroups.com> wrote:
>
> What I'm struggling with is the following:
>
> How do I cimport from different sources (runtime dependent probably
> won't work, but compile time dependent would already be nice). At the
> moment my package just pulls in cysignals during pip install and if this
> does not work it replaces those functions by standard allocation functions.
>
> Is there a better solution to do this?

What do you mean by "cimport from different sources"? If you are
already working on this could you link to the source?

If this is about my idea to make the memory allocation functions
"pluggable" that would be done by setting some function pointers to
them, which would be done by third-party Cython code using
MemoryAllocator (there would need to be a C method for setting them).

> On 10.03.21 15:27, E. Madison Bray wrote:
> > On Sat, Feb 13, 2021 at 2:16 AM Matthias Koeppe
> > <matthia...@gmail.com> wrote:
> >> On Friday, February 12, 2021 at 2:35:45 PM UTC-8 vdelecroix wrote:
> >>> Why make it part of cysignals? The purpose of the memory allocator
> >>> is quite distinct from cysignals. Merging them look artificial to me.
> >>
> >> Really? MemoryAllocator is just a wrapper class around cysignals.memory
> > Was there ever any action on this? MemoryAllocator does seem like a
> > useful thing to have as an independent package. It's basically a more
> > sophisticated version of the SomeMemory recipe given in the Cython
> > docs: https://cython.readthedocs.io/en/latest/src/tutorial/memory_allocation.html
> >
> > I agree that it's distinct from cysignals. It doesn't necessarily
> > have to depend on cysignals, and could have a hook interface to allow
> > different memory allocation functions. It just happens to use the
> > ones from cysignals since Sage already uses cysignals, and its memory
> > functions have the advantage of being interrupt-safe.
> >
> > I would continue using cysignals by default for an initial
> > implementation, but if anyone had a need for it, it would be possible
> > to make this work with cysignals as an optional dependency as well.
> >
>

> --
> You received this message because you are subscribed to the Google Groups "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/58a5c332-7331-8433-941e-b73f7090d608%40gmail.com.

[Jonathan Kliem]

Here is my project (I just made the repository public):
https://github.com/kliem/PyKPartiteKClique/tree/main/

The source folder is kpkc

As you can see, I completely copied memory_allocator*

This should be moved to its own project eventually.

Instead of cimporting directly from cysignals (which may fail, if not
present), I cimport from kpkc.cysignals

There are two files for this. kpkc/cysignals.pyx which is a plain
wrapper about the cysignals functions (just cimporting did not work).

If cysignals is not present, I instead compile kpkc/cysignals_backup.pyx
as module kpkc.cysignals.

Of course, you need to reinstall the project, if you install cysignals
afterwards.

[jonatha...@googlemail.com]

This approach might not be pretty, but it seems to work fine.

I can use cysignals on cygwin, linux, mac and not use it on windows without cygwin.

However, it would be much nicer to allow pip installing cysignals on windows without cygwin in the first place. This would simplify this a lot.

[jonatha...@googlemail.com]

https://github.com/kliem/memory_allocator

I started MemoryAllocator as a seperate project. I think it is almost ready to push to PyPI. Any opinions? Do I need permission to publish it like this? Should this be moved into a different repository? (E.g. https://github.com/sagemath/memory_allocator).

It symlinks `cysignals/src/cysignals/memory.pxd`. Thus it only depends on `sig_block`/`sig_unblock` from cysignals. This will be used, if cysignals is found at time of setup. Otherwise, `sig_block`/`sig_unblock` will be replaced by functions doing nothing. Using `sig_block`/`sig_unblock` only seems to make sense, if you have cysignals anyway.

Disadvantage of this approach is of course, that you need to make sure that you install cysignals before memory_allocator manually.

[Vincent Delecroix]

+1 for having it inside https://github.com/sagemath

I think it is desirable to have the possibility to enable or disable
cysignals manually. In other words
enable + "no cysignals present" -> error
enable + "cysignals present" -> compilation with sigs
disable + "whatever" -> compilation without sigs

I thought cysignals would be Windows compatible after

https://github.com/sagemath/cysignals/pull/104

Though the project seems to be abandoned. What a shame.

Vincent

[Jonathan Kliem]

On Mar 29, 2021 3:23 PM, Vincent Delecroix <20100.d...@gmail.com> wrote:
>
> +1 for having it inside https://github.com/sagemath
>
> I think it is desirable to have the possibility to enable or disable
> cysignals manually. In other words
>    enable + "no cysignals present" -> error
>    enable + "cysignals present" -> compilation with sigs
>    disable + "whatever" -> compilation without sigs

I agree that this would be nice. However, I don't know how to achieve this. One thing that one could try, is to add python functions to cysignals that typecast the addresses of sig_block/sig_unblock to a python int. It's definitely not clean, but it might work. E.g. you can check for the presence of cysignals in __init__.pxd with try … except this way.

>
> I thought cysignals would be Windows compatible after
>
> https://github.com/sagemath/cysignals/pull/104
>
> Though the project seems to be abandoned. What a shame.

I've seen it, but I haven't taken the time to understand this. I agree that it would be best to have cysignals installable on windows without cygwin.

However, if you want the memory allocator and don't have cysignals installed already, you probably don't have a use case for it.

> You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/6u1VuXJE7wE/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/2d7f6a4e-59cf-5c25-4cc3-ae55280d09e6%40gmail.com.

[Matthias Koeppe]

On Monday, March 29, 2021 at 6:25:28 AM UTC-7 vdelecroix wrote:

I think it is desirable to have the possibility to enable or disable
cysignals manually. In other words
enable + "no cysignals present" -> error
enable + "cysignals present" -> compilation with sigs
disable + "whatever" -> compilation without sigs

This suggestion is not compatible with modern Python packaging (PEP-517/518). There is no such thing as an "optional build dependency".

 

[Jonathan Kliem]

On Mar 29, 2021 5:26 PM, Matthias Koeppe <matthia...@gmail.com> wrote:
>
> On Monday, March 29, 2021 at 6:25:28 AM UTC-7 vdelecroix wrote:
>>
>> I think it is desirable to have the possibility to enable or disable
>> cysignals manually. In other words
>> enable + "no cysignals present" -> error
>> enable + "cysignals present" -> compilation with sigs
>> disable + "whatever" -> compilation without sigs

I misunderstood this.

>
>
> This suggestion is not compatible with modern Python packaging (PEP-517/518). There is no such thing as an "optional build dependency".
>

Is it acceptable to build different versions depending on whether a package is found?

In the end the sig_block/sig_unblock is a bit of overkill anyway for MemoryAllocator. I wouldn't consider it clean to allocate memory with it inside of  sig_on / sig_off.

>  
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/6u1VuXJE7wE/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/c8cff2c8-6c9c-46e7-a9e8-9d5cf390aac8n%40googlegroups.com.

[Matthias Koeppe]

On Monday, March 29, 2021 at 9:21:05 AM UTC-7 jonatha...@googlemail.com wrote:

On Mar 29, 2021 5:26 PM, Matthias Koeppe <matthia...@gmail.com> wrote: 

> This suggestion is not compatible with modern Python packaging (PEP-517/518). There is no such thing as an "optional build dependency".

Is it acceptable to build different versions depending on whether a package is found?

You don't find an installed package at build time. Build isolation provides a fixed set of packages -- that you define in pyproject.toml [build-system] requires. See e.g. https://snarky.ca/what-the-heck-is-pyproject-toml/

[Jonathan Kliem]

I propose removing the sig_block / sig_unblock from MemoryAllocator alltogether.

I think it is really bad practice to put an extension class method into sig_on / sig_off and this would be the only use case that this is good for anyway (I doubt that this is safe in the first place -- MemoryAllocator does not use sig_block / sig_unblock to assure that the memory pointers are stored properly). I think the cysignals allocation functions are mostly used, because they check, whether the allocation was successful (and raise an error if not).

Thank you for providing the reference.

I think in case of a pxd files things are a bit different. If someone uses cysignals and memory_allocator in the same place, e.g.

cdef MemoryAllocator mem = MemoryAllocator()
sig_on()
foo = mem.malloc(1000)
sig_off()

this would have to recompile the corresponding pxd file. So one could check in that pxd file, whether cysignals is present or not. So I think this could be set up, such that any project cimporting from memory_allocator that requires cysignals has memory_allocator with cysignals support. As noted above, I would vote for dropping the sig_block / sig_unblock in MemoryAllocator alltogether.

--
You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/6u1VuXJE7wE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/c3abeea5-8413-479b-8303-74c5cbf0159cn%40googlegroups.com.

[Matthias Koeppe]

On Monday, March 29, 2021 at 12:08:36 PM UTC-7 jonatha...@googlemail.com wrote:

I think in case of a pxd files things are a bit different. If someone uses cysignals and memory_allocator in the same place, e.g.

cdef MemoryAllocator mem = MemoryAllocator()
sig_on()
foo = mem.malloc(1000)
sig_off()

this would have to recompile the corresponding pxd file. So one could check in that pxd file, whether cysignals is present or not. So I think this could be set up, such that any project cimporting from memory_allocator that requires cysignals has memory_allocator with cysignals support. 

Right. If an importing project declares both memory_allocator and cysignals as "build-system requires" in pyproject.toml, then it can make use of both; so conditionalization memoryallocator.pxd based on cython_compile_time_env does make sense.

[Jonathan Kliem]

I didn't figure this out until after you showed me the reference.

conditionalization based on cython_compile_time_env might be hard though. There is no such thing as

try:
    from cysignals.signals cimport sig_on, sig_off
except:
    from .signals_backup cimport sig_on, sig_off

One thing that one may try is basically the following:

cdef void (*sig_block_pt)()
cdef void (*sig_unblock_pt)()

if has_cysignals:
    # replace sig_block, sig_unblock properly
else:
    # replace by pass-functions

However, this probably needs an update of cysignals, as we need a way to access the addresses of sig_block/sig_unblock on runtime (which is the cython_compile_time_env).

The question remains, is it worth it? Do we need/want sig_block/sig_unblock for MemoryAllocator?

--
You received this message because you are subscribed to a topic in the Google Groups "sage-devel" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sage-devel/6u1VuXJE7wE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sage-devel+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/f619d534-43b8-4fc9-aaaf-91d2db52a4b6n%40googlegroups.com.

[jonatha...@googlemail.com]

Are there objections of adding this new standard package, which consists of the code from `ext/memory_allocator.*`?

This is now https://trac.sagemath.org/ticket/31591.

I would like to put it on `github.com/sagemath` but need either help or permissions to do so.

The new package does not depend on cysignals anymore for the following reasons:

- Cysignals is not installable on windows without cygwin.

- Cysignals is used in case of  `sig_on`/`sig_off` around `MemoryAllocator` allocation methods. However this is unsafe and incorrect usage of `sig_on`/`sig_off` as only pure C/C++ code may be executed within and extension class cdef methods that involve `self` are not pure C/C++ (they also do not allow to be declared `nogil`). Currently, such usage is not the case in sage.

If you have objections, concerns or other comments, please put them here or directly on #31591.

Thank you,

Jonathan

[Dima Pasechnik]

On Wed, 26 May 2021 at 08:04, 'jonatha...@googlemail.com' via sage-devel <sage-...@googlegroups.com> wrote:

Are there objections of adding this new standard package, which consists of the code from `ext/memory_allocator.*`?

This is now https://trac.sagemath.org/ticket/31591.

I would like to put it on `github.com/sagemath` but need either help or permissions to do so.

I’ll fix this, so that it is hosted there, and get you the needed permissions.

Dima

You received this message because you are subscribed to the Google Groups "sage-devel" group.


To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.


To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/eca09c24-6d63-494f-8f4f-0529d5db7974n%40googlegroups.com.

[Dima Pasechnik]

On Wed, May 26, 2021 at 8:04 AM 'jonatha...@googlemail.com' via
sage-devel <sage-...@googlegroups.com> wrote:
>
> Are there objections of adding this new standard package, which consists of the code from `ext/memory_allocator.*`?
>
> This is now https://trac.sagemath.org/ticket/31591.

as it's just code refactoring, I don't see why this should be an issue.

> You received this message because you are subscribed to the Google Groups "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/sage-devel/eca09c24-6d63-494f-8f4f-0529d5db7974n%40googlegroups.com.

[Travis Scrimshaw]

I don't see why either. I just wanted to check here to see if someone did have an issue before proceeding.

Best,

Travis