MPI and private methods in octave::load_save_system

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

MPI and private methods in octave::load_save_system

kingcrimson
Hi,

In Octave 5 the functions

  do_load, do_save, read_binary_data, write_binary_data

from load-save.cc have been transformed into private methods of the class
 
  octave::load_save_system
 
The MPI package relies on these functions to Send/Receive variables as MPI messages.

Would it be possible to make these methods public so they can be accessed by DLD functions?


c.




Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

John W. Eaton
Administrator
On 2/13/19 11:11 AM, c. wrote:

> Hi,
>
> In Octave 5 the functions
>
>    do_load, do_save, read_binary_data, write_binary_data
>
> from load-save.cc have been transformed into private methods of the class
>    
>    octave::load_save_system
>    
> The MPI package relies on these functions to Send/Receive variables as MPI messages.
>
> Would it be possible to make these methods public so they can be accessed by DLD functions?

Possibly, but I'd like to know how these functions are being used.  I
don't see any of those functions being used in the source files found at
http://hg.code.sf.net/p/octave/mpi at revision

   changeset:   49:506cbb214007
   tag:         tip
   user:        Rafael Laboissiere <[hidden email]>
   date:        Mon Jan 01 17:14:51 2018 +0100
   summary:     Fix speeling errors in doc strings (bug #52766)

Am I looking at the correct package?

BTW, that's the best commit message I've seen in a while.  :-)

jwe



Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson


> On 13 Feb 2019, at 17:35, John W. Eaton <[hidden email]> wrote:
>
> On 2/13/19 11:11 AM, c. wrote:
>> Hi,
>> In Octave 5 the functions
>>   do_load, do_save, read_binary_data, write_binary_data
>> from load-save.cc have been transformed into private methods of the class
>>      octave::load_save_system
>>   The MPI package relies on these functions to Send/Receive variables as MPI messages.
>> Would it be possible to make these methods public so they can be accessed by DLD functions?
>
> Possibly, but I'd like to know how these functions are being used.  I don't see any of those functions being used in the source files found at http://hg.code.sf.net/p/octave/mpi at revision
>
>  changeset:   49:506cbb214007
>  tag:         tip
>  user:        Rafael Laboissiere <[hidden email]>
>  date:        Mon Jan 01 17:14:51 2018 +0100
>  summary:     Fix speeling errors in doc strings (bug #52766)
>
> Am I looking at the correct package?

No, that version of the package is not maintained anymore, the code in there is not compatible with Octave 4.4 or later [1].
Current development goes on in github [2] (actually I just now moved it from bitbucket ...)

The relevant code is here:

https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L65
https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L67

https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L83
https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L90


>
> BTW, that's the best commit message I've seen in a while.  :-)

Kudos to Raphael! Unfortunately fixing typos in a broken package is not very useful :(

>
> jwe


[1] see, e.g.: http://lists.gnu.org/archive/html/octave-maintainers/2018-06/msg00004.html
[2] https://github.com/carlodefalco/octave-mpi


Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson


> On 13 Feb 2019, at 19:09, [hidden email] wrote:
>
> No, that version of the package is not maintained anymore, the code in there is not compatible with Octave 4.4 or later [1].
> Current development goes on in github [2] (actually I just now moved it from bitbucket ...)
>
> The relevant code is here:
>
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L65
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L67
>
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L83
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L90
>


BTW, if this change were to be made, would it be on some stable or default?

c.
Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

John W. Eaton
Administrator
On 2/14/19 11:15 AM, [hidden email] wrote:

>
>
>> On 13 Feb 2019, at 19:09, [hidden email] wrote:
>>
>> No, that version of the package is not maintained anymore, the code in there is not compatible with Octave 4.4 or later [1].
>> Current development goes on in github [2] (actually I just now moved it from bitbucket ...)
>>
>> The relevant code is here:
>>
>> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L65
>> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L67
>>
>> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L83
>> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Recv.cc#L90
>>
>
>
> BTW, if this change were to be made, would it be on some stable or default?

I'd be willing to make minor changes on stable (moving a declaration
from private to public, for example) if necessary.  As we discussed on
IRC, I was considering making more extensive changes to avoid making
some messy interfaces public again, but I'm not sure that we should do
that.  We were talking about providing functions like

   octave_value load_save_system::read_value (std::istream& is,
std::string& name, bool& global);

and

   void load_save_system::write_value (std::ostream& os, const
octave_value& val, const std::string& name, bool global, const
load_save_format& fmt, bool save_as_floats);

but when I started looking at what would be required to extract the the
read_value function from the load_vars function it was not as simple as
I assumed.  Some of the functions that ultimately write the data require
extra info that wouldn't be in the above interface.  We should probably
clean all of that up and use consistent interfaces.  But I think the
changes are too much to make just before the release.

Also on IRC, you said the functions that are required are

   write_header
   save_binary_data
   read_binary_file_header
   read_binary_data

The save_binary_data and read_binary_data functions are public and
declared in ls-oct-binary.h.  If you know that the data on the stream is
Octave binary data using a particular float format, do you really need
to read and write the header, or can you omit that?  If you can omit it,
then I don't think we need to change anything before the release.  If
you do need it, what is it used for?  If it really is necessary, then I
would consider making those functions public for this release.

Longer term, however, I think it would be good to come up with better
and more consistent interfaces.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson
Hi John,

Thanks for looking into this.

> On 15 Feb 2019, at 23:29, John W. Eaton <[hidden email]> wrote:
>
> I'd be willing to make minor changes on stable (moving a declaration from private to public, for example) if necessary.  As we discussed on IRC, I was considering making more extensive changes to avoid making some messy interfaces public again, but I'm not sure that we should do that.  We were talking about providing functions like
>
>  octave_value load_save_system::read_value (std::istream& is, std::string& name, bool& global);
>
> and
>
>  void load_save_system::write_value (std::ostream& os, const octave_value& val, const std::string& name, bool global, const load_save_format& fmt, bool save_as_floats);
>
> but when I started looking at what would be required to extract the the read_value function from the load_vars function it was not as simple as I assumed.  Some of the functions that ultimately write the data require extra info that wouldn't be in the above interface.  We should probably clean all of that up and use consistent interfaces.  But I think the changes are too much to make just before the release.

I totally agree this is not a good moment to make invasive changes, I was actually going to say son IRC before my web client stopped working ;)

> Also on IRC, you said the functions that are required are
>
>  write_header
>  save_binary_data
>  read_binary_file_header
>  read_binary_data

Yes, I thank that is all that is needed in the MPI package.

It is also an useful functionality to be able to read and write values to file without using the interpreter workspace,
in order to import/export data in Octave format from independent applications. I use this often in standalone C++ or F90
applications in order to do pre- and post-processing with Octave while computation is done elsewhere.

I once documented how to do this here : https://wiki.octave.org/Fortran

That example doesn't work now, but it is quite easy to update. To make this work,
the do_load method also needs to be public.


> The save_binary_data and read_binary_data functions are public and declared in ls-oct-binary.h.  If you know that the data on the stream is Octave binary data using a particular float format, do you really need to read and write the header, or can you omit that?  If you can omit it, then I don't think we need to change anything before the release.  If you do need it, what is it used for?  If it really is necessary, then I would consider making those functions public for this release.

I'm not sure whether they are actually needed, I'll look into this this evening.

> Longer term, however, I think it would be good to come up with better and more consistent interfaces.

It would be great to have a stable and documented API for this.

> jwe

Thanks again,
c.


Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson


> On 16 Feb 2019, at 09:13, [hidden email] wrote:
>
>> Longer term, however, I think it would be good to come up with better and more consistent interfaces.
>
> It would be great to have a stable and documented API for this.

I also think it would be nice, on the long term, to have the save-load functions/classes to be independent of the interpreter.
c.
Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

Andreas Weber-6
In reply to this post by kingcrimson
Hi Carlo,

Am 13.02.19 um 19:09 schrieb [hidden email]:

>> On 13 Feb 2019, at 17:35, John W. Eaton <[hidden email]> wrote:
>> Am I looking at the correct package?
>
> No, that version of the package is not maintained anymore, the code in there is not compatible with Octave 4.4 or later [1].
> Current development goes on in github [2] (actually I just now moved it from bitbucket ...)
>
> The relevant code is here:
>
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L65
> https://github.com/carlodefalco/octave-mpi/blob/master/src/MPI_Send.cc#L67

Is there any possibility to include a message and/or a link to the
current development?


Currently it's impossible to realize form
https://octave.sourceforge.io/mpi/index.html that the linked repository
is dead.

-- Andy

Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

Olaf Till-2
In reply to this post by John W. Eaton
On Fri, Feb 15, 2019 at 05:29:26PM -0500, John W. Eaton wrote:
>   write_header
>   save_binary_data
>   read_binary_file_header
>   read_binary_data

Only as a side note, for data exchange involving only code of a single
package, it should be possible to do without the above, as e.g. in
current parallel package:

https://sourceforge.net/p/octave/parallel/ci/default/tree/src/minimal-load-save.cc

Olaf

--
public key id EAFE0591, e.g. on x-hkp://pool.sks-keyservers.net

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson


> On 17 Feb 2019, at 20:46, Olaf Till <[hidden email]> wrote:
>
> On Fri, Feb 15, 2019 at 05:29:26PM -0500, John W. Eaton wrote:
>>  write_header
>>  save_binary_data
>>  read_binary_file_header
>>  read_binary_data
>
> Only as a side note, for data exchange involving only code of a single
> package, it should be possible to do without the above, as e.g. in
> current parallel package:
>
> https://sourceforge.net/p/octave/parallel/ci/default/tree/src/minimal-load-save.cc
>
> Olaf

Olaf,

Indeed, extracting the code that has been made private in core
and duplicating it in the package is a solution, but I really
wanted to avoid this.

Also, as far as I remember, there is no reliable way of exporting
C++ headers and libraries from one package so that others can
use it, so if multiple packages use this functionality the code
must be duplicated multiple times.

I think having a public API to do this in core is the best solution,
even more so if the same code is in multiple packages.

c.



Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson
In reply to this post by Andreas Weber-6


> On 16 Feb 2019, at 10:36, Andreas Weber <[hidden email]> wrote:
>
> Is there any possibility to include a message and/or a link to the current development?
>
>
> Currently it's impossible to realize form https://octave.sourceforge.io/mpi/index.html that the linked repository is dead.
>
> -- Andy

Yes, it is possible, but it is not up to me to decide.

It was proposed to do so a few times over the last five years or so [1],
but the OF maintainers at the time opposed it.

c.

[1] see, e,g., http://lists.gnu.org/archive/html/octave-maintainers/2018-06/msg00004.html
Reply | Threaded
Open this post in threaded view
|

Re: MPI and private methods in octave::load_save_system

kingcrimson
In reply to this post by kingcrimson


> On 16 Feb 2019, at 09:13, [hidden email] wrote:
>
>>
>> The save_binary_data and read_binary_data functions are public and declared in ls-oct-binary.h.  If you know that the data on the stream is Octave binary data using a particular float format, do you really need to read and write the header, or can you omit that?  If you can omit it, then I don't think we need to change anything before the release.  If you do need it, what is it used for?  If it really is necessary, then I would consider making those functions public for this release.
>
> I'm not sure whether they are actually needed, I'll look into this this evening.

Indeed, if I specify manually the float format to be native_foat_format, the header can be omitted,
with this change the MPI pacakge seems to work :

  https://github.com/carlodefalco/octave-mpi/commit/81ea87a9a820ddeacceb219203974c95717baadc

I think the reason for not specifying manually the float_format was that MPI is expected, in priciple,
to work also on heterogeneous clusters.

I don't think it is a severe limitation if this won't work though (I had never tested communication
between different architectures and I am not sure it ever worked anyway).

So for MPI to work there is no need to do any urgent modification on the stable branch before the 5.1 release,
I'll make a relese of the package as soon as Octave 5.1.0 is out.

Still I think it would be useful to have  methods

  read_*_file_header
  write_header

and possibly

  get_file_format

made public, would it be OK to do that in 5.2?

As a last question  why cannot load_save_system::load_vars (..) be static?

c.