bruceravel / demeter

Process and analyze X-ray Absorption Spectroscopy data using Feff and either Larch or Ifeffit.
http://bruceravel.github.io/demeter
Other
67 stars 32 forks source link

Larch Server refactor, especially for Mac OSX #40

Closed newville closed 7 years ago

newville commented 8 years ago

First: this is NOT READY TO MERGE!!! It needs more testing, especially on non-OSX systems, and perhaps some real discussion of design. This PR is to start the conversation. This now seems to work on Mac OSX without hanging!

Basically, this uses a separate LarchServer.pm module to find and start the Larch server. The Larch.pm imports this server and creates a client connecting to it. This might require Larch past 0.9.30 (so, I'll try to release 0.9.31 this week, but it is the beginning of the run, and I'd like to test Margaret's XRD code more).

It does address #29, as each LarchServer will start with the next unused port.

Some discussion topics:

  1. should the server start with "import LarchServer", or should there be an explicit function to start the server?
  2. the demeter clients should shutdown the server (Larch::shutdown_server();) when exiting, but I wasn't sure where that should go. Like, a plain Demeter script, not just the GUIs should do that too. Is there a magic END{} block in Perl that is run at the end of a process, or is that at the end of import?
bruceravel commented 8 years ago

Is there a magic END{} block in Perl that is run at the end of a process, or is that at the end of import?

http://perldoc.perl.org/perlmod.html#BEGIN%2c-UNITCHECK%2c-CHECK%2c-INIT-and-END

bruceravel commented 8 years ago

the demeter clients should shutdown the server (Larch::shutdown_server();) when exiting, but I wasn't sure where that should go. Like, a plain Demeter script, not just the GUIs should do that too.

I believe this could go in an END block in Demeter.pm. That way scripts and GUIs and whatnot would do the right thing automagically.

There is one other END block used in Demeter. END blocks are run in LIFO order, but order shouldn't matter between what that does and shutting down the server.

newville commented 8 years ago

@bruceravel, Perhaps the END{} block should go in LarchServer.pm? The module that starts it should also stop it? That way, it's sort of outside of even Demeter.

bruceravel commented 8 years ago

Perhaps the END{} block should go in LarchServer.pm? The module that starts it should also stop it? That way, it's sort of outside of even Demeter.

Good point. I agree. Try that. As best as I understand END blocks, that should work just fine and wouldn't require any logic to be sure that Larch was selected as the back end.

bruceravel commented 8 years ago

should the server start with "import LarchServer", or should there be an explicit function to start the server?

I'm not certain I see a use case for an explicit function. Demeter either tries to use Larch or it doesn't.

newville commented 8 years ago

@bruceravel it looks like a simple END{} block in LarchServer.pm works perfectly to stop the larch_server process that it started. That should allow multiple, independent larch_servers / (athena + artemis, even multiple users) on a single machine and not (normally) leave idle larch servers around. I think that means the "kill larch server due to inactivity" can go up from 3 to 30 days. Feeling much better about this, but haven't tested this new branch on Windows yet.

bruceravel commented 8 years ago

Neat-o!

newville commented 8 years ago

@bruceravel OK, with one fix to set username, this branch now works on Windows, at least using the master branch of Larch and Anaconda Python.

Actually, it might need better testing on Linux -- I tried exactly one Linux system.

I'm hoping to push Larch 0.9.31 with the needed version of Larch_server early next week. At that point, I think it will be ready for use with this branch of Demeter.

newville commented 8 years ago

@bruceravel OK, this is much closer. Using RPC::XML::Client and re-factoring how the "next available larch server" is found and launched seems to solve the problem. In addition, a self-test in Larch.pm (which should be converted to a proper test) will run through many, many processing loops without hanging. Of course, using RPC::XML::Client also means that everything about how the data is transferred has changed.

Most stuff works, but there are some processing issues yet to be resolved. Specifically, I get errors from Mu.pm, that I do not understand:

Use of uninitialized value in sprintf at /opt/local/lib/perl5/site_perl/5.24/darwin-thread-multi-2level/Demeter/Data/Mu.pm line 326.

and also at lines 353 and 555. There are also a fair number of tests that fail with "perl Build test". Many of these come from Demeter/Get.pm and Demeter/Data.pm, so I suspect are differences in the return from Larch::get_array_data, but this needs more investigating.

Still not ready to merge, but if you have time to comment on this or see if there are any obvious problems with the Perl, let me know.

newville commented 7 years ago

Working better on OSX/Linux but a few outstanding issues:

  1. failures with import chi(k) data -- not sure why, but it says that I0 contains a value of 0, even though "chi(k)" is selected. Works with Ifeffit backend.

  2. Larch.pm fails to import on Windows, Perl 5.18, with

C:\Users\newville\Documents\GitHub\demeter>perl -e 'use Larch;' Can't find string terminator "'" anywhere before EOF at -e line 1.

C:\Users\newville\Documents\GitHub\demeter>perl -e "use Larch;" syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 136, near "$dat{class" Global symbol "$class" requires explicit package name at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Lar ch.pm line 138. Global symbol "$class" requires explicit package name at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Lar ch.pm line 140. syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 141, near "$dat{value" syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 142, near "$dat{dtype" Global symbol "$dat" requires explicit package name at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch .pm line 143. syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 143, near "$dat{shape" syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 143, near "->value}" Global symbol "$value" requires explicit package name at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Lar ch.pm line 144. syntax error at C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm line 145, near "}" C:/Users/newville/AppData/Roaming/DemeterPerl/perl/site/lib/Larch.pm has too many errors. Compilation failed in require at -e line 1. BEGIN failed--compilation aborted at -e line 1.

C:\Users\newville\Documents\GitHub\demeter>perl -v

This is perl 5, version 18, subversion 2 (v5.18.2) built for MSWin32-x86-multi-thread-64int

Copyright 1987-2013, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on this system using "man perl" or "perldoc perl". If you have access to the Internet, point your browser at http://www.perl.org/, the Perl Home Page.

bruceravel commented 7 years ago

So, it seems like the easiest path forward is to merge this PR and treat the problems identified on Nov 28 and Nov 29 as issues. Thanks for all of this!

newville commented 7 years ago

@bruceravel OK -- I'll make make another merge request for things done yesterday but not pushed.

bruceravel commented 7 years ago

@newville

Still need to give this a try on Windows -- and look into the problem you are reporting -- but that likely won't happen today.