Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the macros for communication with a remote R server #214

Merged
merged 2 commits into from
May 31, 2016
Merged

Add the macros for communication with a remote R server #214

merged 2 commits into from
May 31, 2016

Conversation

cubranic
Copy link
Contributor

@cubranic cubranic commented Jun 8, 2015

Add the macros for communication with a remote R server

@jwj61
Copy link
Member

jwj61 commented Jun 16, 2015

Since this request is to pg, I am leaving it to Mike/Davide/Geoff to decide about (should the new file go here, or in the OPL macros?).

@dlglin
Copy link
Member

dlglin commented Jun 16, 2015

This actually raises the larger question of where contributed macros should live, as some are currently in the pg directory, and some are in the OPL.

On Jun 16, 2015, at 2:00 PM, John Jones <[email protected]mailto:[email protected]> wrote:

Since this request is to pg, I am leaving it to Mike/Davide/Geoff to decide about (should the new file go here, or in the OPL macros?).


Reply to this email directly or view it on GitHubhttps://github.com//pull/214#issuecomment-112551521.

@mgage
Copy link
Member

mgage commented Jun 16, 2015

Danny,

It probably depends. In this case -- where the macros will be widely used (eventually) I think importing these macros into pg is appropriate. The macros in the OPL are also searched if the file they are in is placed in the macro search path in defaults.conf (or the override configurations). That's probably the best place for macros associated with one or a small group of problems from a specific school.

We have in the past moved macros from an OPL location into the pg/macros file as they became more widely used. We don't have a specific policy for doing this yet.

@goehle
Copy link
Member

goehle commented Jun 22, 2015

A couple of questions. (Sorry for getting to this so late, I was away for a couple weeks.)

One for @cubranic Do you have a pg file I can use to test this macro?

One for @mgage. This macro file requires additional configuration to use. Specifically you have to specify $pg{specialPGEnvironmentVars}{Rserve} = {host => "localhost"}; in a conf file. Does this mean that this macro should go in the OPL or are we ok with having macros in PG that dont work "out of the box".

@cubranic
Copy link
Contributor Author

@goehle Here is a PG file from our problem library: https://github.com/ubc/webwork-open-problem-library/blob/master/OpenProblemLibrary/UBC/STAT/STAT300/hw01/stat300_hw01_q02.pg

Also, the reason the additional configuration has to be done that way is so the macro can have a soft-fail with a reasonable message if it's not present. This was requested by @jwj61 and the way I implemented it was proposed on the mailing list a while back (probably around a year).

@goehle
Copy link
Member

goehle commented Jun 23, 2015

That makes sense. Since there needs to be an additional configuration line in localOverrides.conf could you make a pull request to webwork2 that has a commentented out configuration line in localOverrides.conf.dist along with a couple of lines of documentation explaining what it is and why you would set it.

@goehle
Copy link
Member

goehle commented Jun 23, 2015

I'm having trouble getting this to run. I've installed Statistics::R::IO::Rserve, and I installed an rserver and have it running. When I try to run the problem I get

Can't locate object method "read" via package "IO::File" at /usr/local/share/perl/5.14.2/Statistics/R/IO/Rserve.pm line 38

Is there other configuration that I"m missing?

Some other things you will need to add to the webwork2 pull.

  • You should add Statistics::R::IO::Rserve to check modules (or at least write in localOverrides that it needs to be installed in order for the problems to work)
  • You should add Statistics::R::IO::Rserve to the ${pg}{modules} list in defaults.config so that the package can be used in pg.

@cubranic
Copy link
Contributor Author

@goehle Sorry, I think I left you out of an email in which I described the pull request (it was part of a separate discussion on OPL). There are indeed several modules that need to be in ${pg}{modules}, we have the list in our installation guide.

@dlglin
Copy link
Member

dlglin commented Jun 24, 2015

My preference for add-ons like this would be to have a separate config file which includes all of the static configuration needed, and then a small section in localOverrides.conf referring to it, and including the server-specific settings. Something like the following:

To enable the Rserve integration, uncomment the following lines, and set the location of your R server:

$pg{specialPGEnvironmentVars}{Rserve} = {host => "localhost”};

include(“conf/Rserve.conf”);

This way, the user is only bothered with the things that they need to set.

Danny

On Jun 24, 2015, at 12:42 PM, Davor Cubranic <[email protected]mailto:[email protected]> wrote:

@goehlehttps://github.com/goehle Sorry, I think I left you out of an email in which I described the pull request (it was part of a separate discussion on OPL). There are indeed several modules that need to be in ${pg}{modules}, we have the list in our installation guidehttp://wiki.ubc.ca/Documentation:WeBWork/The_WeBWorKiR_Project:_Integrating_WeBWorK_with_R/Installation_guide.


Reply to this email directly or view it on GitHubhttps://github.com//pull/214#issuecomment-114976394.

@goehle
Copy link
Member

goehle commented Jun 24, 2015

I'm not sure there is any static information other than the server address. If its more than 5 or 10 lines of configuration then another file is definitely worth it. Right now its just the one (and some modules that need to be added to the existing pg module list.)

However ... the list you have on the installation guide includes IO::File. This is not safe for us to include in the OK list for PG because pg problems could use it to open files on the server. I'm not sure what to do at this point. It can be put in the OPL macros folder without any real issues, but I'm getting a little wary about adding it to pg.

@mgage
Copy link
Member

mgage commented Jun 24, 2015

What exactly is used in IO::File? Sometimes we can construct our own module that can be included and which allows only restricted
access to the IO::File (which does not need to be included in Safe). This works well on older .pm modules that tend to do all of their setup at compile time. If IO::File does a lot of runtime evaluation then it will be a pain in the neck.

Take care,
Mike

On Jun 24, 2015, at 4:13 PM, Geoff Goehle [email protected] wrote:

I'm not sure there is any static information other than the server address. If its more than 5 or 10 lines of configuration then another file is definitely worth it. Right now its just the one (and some modules that need to be added to the existing pg module list.)

However ... the list you have on the installation guide includes IO::File. This is not safe for us to include in the OK list for PG because pg problems could use it to open files on the server. I'm not sure what to do at this point. It can be put in the OPL macros folder without any real issues, but I'm getting a little wary about adding it to pg.


Reply to this email directly or view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_pg_pull_214-23issuecomment-2D114997086&d=AwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=8GAxLeDMTN5oF79R3AwgJiUXb6TVJAEgDgzONJNGKkU&m=i5FF_-SsQwezLkNMhzVY9TVV3xxpJ1ZY5Y_fjZsRJto&s=5GE-nvPhDXFiexq7kfzQl-5zb4lnGcOKe9CJ8KwhTOc&e=.

@cubranic
Copy link
Contributor Author

Maybe you can clarify for me an issue that led me to including IO::File in that list. It's not used in the macro file itself, only in the Statistics::R::IO distribution. Yet, without IO::File in ${pg}{modules}, I was getting errors from WWSafe. Is there no way around it?

@goehle
Copy link
Member

goehle commented Jun 24, 2015

Like mike said, sometimes we can create "dummy" packages which will fool Statistics::R::IO into thinking it has access to IO::File, and as long as you don't try to use anything in Statistics::R::IO that actually uses IO::File it can work. However, I think part of the problem is that this is all buried under several levels of abstraction which makes it tough to tell what is exactly being used from the packages.

@cubranic
Copy link
Contributor Author

RserveClient.pl creates an instance of Statistics::R::IO::Rserve, which in turn loads:

use Moose;

use Statistics::R::IO::REXPFactory;
use Statistics::R::IO::QapEncoding;

use Socket;
use IO::Socket::INET ();
use Scalar::Util qw(blessed looks_like_number);
use Carp;

use namespace::clean;

So it doesn't really need it directly, but I guess via IO::Socket::INET.

@mgage
Copy link
Member

mgage commented Jun 24, 2015

The usual problem is that Statistics::R::IO is trying to compile some part of itself at runtime.

That was what was going on with maketext() in Locale — the localization module. That was difficult to
get to run inside of Safe.

Take care,

Mike

On Jun 24, 2015, at 4:36 PM, Geoff Goehle [email protected] wrote:

Like mike said, sometimes we can create "dummy" packages which will fool Statistics::R::IO into thinking it has access to IO::File, and as long as you don't try to use anything in Statistics::R::IO that actually uses IO::File it can work. However, I think part of the problem is that this is all buried under several levels of abstraction which makes it tough to tell what is exactly being used from the packages.


Reply to this email directly or view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_pg_pull_214-23issuecomment-2D115003536&d=AwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=8GAxLeDMTN5oF79R3AwgJiUXb6TVJAEgDgzONJNGKkU&m=xj4OFoJ8kfPjysCiHL5pS74cQ0WjdK1-Kt-6weTEFjo&s=dHssRNLvXtKVaHZfVNYEJbBB2ihaP6a3RKkvqLozMyM&e=.

@goehle
Copy link
Member

goehle commented Jun 24, 2015

Sockets in linux can be created and controled by opening files. Its possible that is how it is being used. If so then its likely integral to the network functionality of the whole thing. Is there a client only version of the package that connects via http or something safer?

@mgage
Copy link
Member

mgage commented Jun 24, 2015

The fact that it has use Moose;
could cause a problem. I believe Moose does a lot of run time setup.
Is there any clue where the problem is occuring? I would expect some error
such as “eval” caught at line ….. when operating inside Safe.

Take care,

Mike

On Jun 24, 2015, at 4:42 PM, Davor Cubranic [email protected] wrote:

RserveClient.pl creates an instance of Statistics::R::IO::Rserve https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cubranic_Statistics-2DR-2DIO_blob_master_lib_Statistics_R_IO_Rserve.pm&d=AwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=8GAxLeDMTN5oF79R3AwgJiUXb6TVJAEgDgzONJNGKkU&m=yfvxSc1rYZU0ifV85N-qdODgVN5FHAlmiluV42uhKA0&s=Me5burJQXuag2Q3exWDZSb7nbzXPz-SLEzoUAVoT40A&e=, which in turn loads:

use Moose;

use Statistics::R::IO::REXPFactory;
use Statistics::R::IO::QapEncoding;

use Socket;
use IO::Socket::INET ();
use Scalar::Util qw(blessed looks_like_number);
use Carp;

use namespace::clean;
So it doesn't really need it directly, but I guess via IO::Socket::INET.


Reply to this email directly or view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_pg_pull_214-23issuecomment-2D115005293&d=AwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=8GAxLeDMTN5oF79R3AwgJiUXb6TVJAEgDgzONJNGKkU&m=yfvxSc1rYZU0ifV85N-qdODgVN5FHAlmiluV42uhKA0&s=CjQfZJMu4fRHnxEts3QFgdm6srzbRDvedmrwKlfZz98&e=.

@cubranic
Copy link
Contributor Author

@mgage This is the error:

ERRORS from evaluating PG file: 
 Can't locate object method "read" via package "IO::File" at /usr/local/share/perl/5.14.2/Statistics/R/IO/Rserve.pm line 38
   Died within Statistics::R::IO::Rserve::fh called at line 306 of /usr/local/share/perl/5.14.2/Statistics/R/IO/Rserve.pm
   from within Statistics::R::IO::Rserve::_send_command called at line 217 of /usr/local/share/perl/5.14.2/Statistics/R/IO/Rserve.pm
   from within Statistics::R::IO::Rserve::eval called at line 127 of [PG]/macros/RserveClient.pl
   from within main::rserve_start called at line 142 of [PG]/macros/RserveClient.pl
   from within main::rserve_eval called at line 46 of (eval 2393)

The line apparently causing it is a call to the print method on the socket handle:

$self->fh->print(pack('V4', $command, length($parameters), 0, 0) .
                     $parameters);

@cubranic
Copy link
Contributor Author

@goehle You're probably thinking of domain sockets (PF_UNIX) -- I don't think any file is created for IP sockets (PF_INET), which I use here.

What I don't see is how is connecting via HTTP any safer. It would still ultimately have to use a socket, would it not?

@goehle
Copy link
Member

goehle commented Jun 24, 2015

I was thinking along the lines of how geogebra or sage connects. (Although
that is all client side so I suppose this is pretty different.) In any
case it looks like the system is actually trying to use IO::File::read,
which can't be in the safe compartment.

On Wed, Jun 24, 2015 at 6:51 PM, Davor Cubranic [email protected]
wrote:

@goehle https://github.com/goehle You're probably thinking of domain
sockets (PF_UNIX) -- I don't think any file is created for IP sockets
(PF_INET), which I use here.

What I don't see is how is connecting via HTTP any safer. It would still
ultimately have to use a socket, would it not?


Reply to this email directly or view it on GitHub
#214 (comment).

@dpvc
Copy link
Member

dpvc commented Jun 24, 2015

What you probably want to do is make your own .pm file in pg/lib that provides an interface to the Statistics::R::IO::Rserve package and that can be included into the Safe compartment. Your .pm file can use IO::File, since it is not itself in the safe compartment, but it would provide a package that can be imported into the safe compartment. That package could be called from PG, and in term call the Statistics::R::IO::Rserve package. Since your package is compiled outside the safe compartment, that would allow access to R through your interface. It might be enough simply to provide a method that calls Statistics::R::IO::Rserve->new() and the rest could be in your .pl file as is. If the new call is done in a .pm file rather than a .pl file, I think you will be OK.

@cubranic
Copy link
Contributor Author

@goehle If you're thinking of AskSage macro, that actually communicates by running an external curl process. It's "safe" because the function is defined in PG.pm, but I can't believe that's the best way to do it.

@cubranic
Copy link
Contributor Author

@dpvc Thanks! Would this file, let's call it lib/Rserve.pm, also be part of this pull request?

@goehle
Copy link
Member

goehle commented Jun 24, 2015

I think its the same basic principle. AskSage is defined outside of the safe compartment in IO.pm (if its the one I'm thinking of) and the function itself is exported to the safe compartment. You would add a lib/Rserve.pm file to this pull request as Davide suggested and add that file to the safe list. Then you wouldn't need to add all of the other modules to the safe list. (Although you should still add them to check_modules.pm)

@dpvc
Copy link
Member

dpvc commented Jun 24, 2015

Yes. You would also add lib/Rserve.pm to the $pg{modules} list (inwebwork2/config/defaults.config) so that your module is loaded, as I assume you did forStatistics::R::IO::Rservefor your current macros. Of course, you would remove any such modules now, and only loadlib/Rserver.pminstead. That library would includeuse Statistics::R` or whatever is needed.

It has been a while since I did this sort of thing, but that is my recollection of how it works. Your lib/Rserve.pm could potentially be as simple as

use Statistics::R::IO::Rserve;

package Rserve;
sub access {Statistics::R::IO::Rserve->new(@_)};

1;

and then your .pl file would use Rserve::access(...) in place of Statistics::R::IO::Rserve->new(...).

@cubranic
Copy link
Contributor Author

That's great, thanks @dpvc and @goehle, I'll update this request as you suggest.

@cubranic
Copy link
Contributor Author

cubranic commented May 6, 2016

I have updated the code (after rebasing it onto current develop) to refactor the interface to "Statistics::R::IO" to lib/Rserve.pm, which is now the only module required inside ${pg}{modules}. (Actually, the required addition is: [qw(Rserve Moose IO::File IO::Handle IO::Seekable)]; without the additional modules, the code was not finding required methods when opening the connection to Rserve.) I tested it, and it works fine.

I have a couple of tidying-up questions at this point:

  1. should I submit a pull request to openwebwork/webwork2 with the required entry for ${pg}{modules} added to defaults.conf?
  2. if the user doesn't have Statistics::R::IO installed, there will be a warning about Rserve.pm on every question render, even if it doesn't use the RserveClient macros. Should I replace use ... with a require in a BEGIN block and check this status in each call into it?
  3. can I squash the various fixup commits in my branch?

@goehle
Copy link
Member

goehle commented May 9, 2016

Unfortunately still requires us to add IO::File to the safe compartment, which we can't do. However after many hours of poking I've found some magic which may fix things. Or at the very least I was able to run the test problem posted above. You should run tests yourself to make sure I'm not missing anything. The main downside is that it requires a change to the Statistics::R::IO::Rserve package. First, add [Rserve IO::Handle Moose] to the module list. Then, make a local copy of Statistics::R::IO::Rserve and add the line

           bless $fh, 'IO::Handle';

to line 30. That will change the $fh generated by socket to be explicitly an instance of IO::Handle.
In particular, the socket handle is being generated by socket and isn't actually a subclass of IO::Handle. There is some magic typecasting going on when you call $fh->read which is turning $fh into an IO::File object. However, since this typecasting happens at runtime, this occurs in the safe compartment which doesn't have access to IO::File.

As for your questions.

  1. Yes, you will need to add these modules to defaults.conf. Once we get this pull request worked out that one will be quick.
  2. In order for this fix to work WeBWorK will have to have a special copy of Statistics::R::IO::Rserve, so there will always be a copy of that module available.
  3. A good commit structure is nice, but not required. :)

@cubranic
Copy link
Contributor Author

cubranic commented May 9, 2016

Thanks for looking into this in such detail Geoff. I’m happy to go with your suggestion, but can you clarify something about the safe compartment for me? When I have an entry such as [Rserve Moose IO::Handle] in ${pg}{modules}, where the entry is a multi-package list, I thought only the very first one is put in the safe compartment and the rest are OK only during the initial load? So there is no danger of user code (i.e., a problem file) being able to directly call either Moose or IO::Handle, e.g., to instantiate a Moose object. (Or open files, as might be the case with IO::File.)

Re your answer #2: I’m the maintainer of Statistics::R::IO, so there is no problem of modifying the official release on CPAN, especially since your change is so minor. In addition, since S::R::IO depends on Moose, unless you want to add Moose as WebWork dependency, we should still probably replace use with require for a more fault-tolerant loading of Rserve.pm

On May 9, 2016, at 2:05 PM, Geoff Goehle [email protected] wrote:

Unfortunately still requires us to add IO::File to the safe compartment, which we can't do. However after many hours of poking I've found some magic which may fix things. Or at the very least I was able to run the test problem posted above. You should run tests yourself to make sure I'm not missing anything. The main downside is that it requires a change to the Statistics::R::IO::Rserve package. First, add [Rserve IO::Handle Moose] to the module list. Then, make a local copy of Statistics::R::IO::Rserve and add the line

       bless $fh, 'IO::Handle';

to line 30. That will change the $fh generated by socket to be explicitly an instance of IO::Handle.

In particular, the socket handle is being generated by socket and isn't actually a subclass of IO::Handle. There is some magic typecasting going on when you call $fh->read which is turning $fh into an IO::File object. However, since this typecasting happens at runtime, this occurs in the safe compartment which doesn't have access to IO::File.

As for your questions.

  1. Yes, you will need to add these modules to defaults.conf. Once we get this pull request worked out that one will be quick.
  2. In order for this fix to work WeBWorK will have to have a special copy of Statistics::R::IO::Rserve, so there will always be a copy of that module available.
  3. A good commit structure is nice, but not required. :)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #214 (comment)

@goehle
Copy link
Member

goehle commented May 9, 2016

That's what I thought too, but I was wrong. Turns out the intended purpose
is if you have multiple packages in one file you would do [mainfile
subpackage1 subpackage2]. The first file is required and imported and the
rest are just imported. I think its mostly just luck that they all end up
being available. In any case they are all shared with the safe compartment
https://github.com/openwebwork/pg/blob/master/lib/WeBWorK/PG/Translator.pm#L298

I was under the impression that use was just BEGIN { require thing; import
thing; } What makes require more fault tolerant?

Also, since you have used Moose, you may be able to answer this. Is it
dangerous to include it in the safe compartment. Its big and complicated
and does dynamic method creation, but unless it allows users to eval code
or access files its probably ok.

Geoff.

On Mon, May 9, 2016 at 5:18 PM, Davor Cubranic [email protected]
wrote:

Thanks for looking into this in such detail Geoff. I’m happy to go with
your suggestion, but can you clarify something about the safe compartment
for me? When I have an entry such as [Rserve Moose IO::Handle] in
${pg}{modules}, where the entry is a multi-package list, I thought only
the very first one is put in the safe compartment and the rest are OK only
during the initial load? So there is no danger of user code (i.e., a
problem file) being able to directly call either Moose or IO::Handle, e.g.,
to instantiate a Moose object. (Or open files, as might be the case with
IO::File.)

Re your answer #2: I’m the maintainer of Statistics::R::IO, so there is no
problem of modifying the official release on CPAN, especially since your
change is so minor. In addition, since S::R::IO depends on Moose, unless
you want to add Moose as WebWork dependency, we should still probably
replace use with require for a more fault-tolerant loading of Rserve.pm

On May 9, 2016, at 2:05 PM, Geoff Goehle [email protected]
wrote:

Unfortunately still requires us to add IO::File to the safe compartment,
which we can't do. However after many hours of poking I've found some magic
which may fix things. Or at the very least I was able to run the test
problem posted above. You should run tests yourself to make sure I'm not
missing anything. The main downside is that it requires a change to the
Statistics::R::IO::Rserve package. First, add [Rserve IO::Handle Moose] to
the module list. Then, make a local copy of Statistics::R::IO::Rserve and
add the line

bless $fh, 'IO::Handle';
to line 30. That will change the $fh generated by socket to be
explicitly an instance of IO::Handle.

In particular, the socket handle is being generated by socket and isn't
actually a subclass of IO::Handle. There is some magic typecasting going on
when you call $fh->read which is turning $fh into an IO::File object.
However, since this typecasting happens at runtime, this occurs in the safe
compartment which doesn't have access to IO::File.

As for your questions.

  1. Yes, you will need to add these modules to defaults.conf. Once we get
    this pull request worked out that one will be quick.
  2. In order for this fix to work WeBWorK will have to have a special
    copy of Statistics::R::IO::Rserve, so there will always be a copy of that
    module available.
  3. A good commit structure is nice, but not required. :)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub <
https://github.com/openwebwork/pg/pull/214#issuecomment-217989601>


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#214 (comment)

@cubranic
Copy link
Contributor Author

cubranic commented May 9, 2016

I was under the impression that use was just BEGIN { require thing; import
thing; } What makes require more fault tolerant?

I can include it in an eval {} block and set a status variable that's checked in Rserve::access to make
sure the package is imported before calling it.

@goehle
Copy link
Member

goehle commented May 9, 2016

Ah, sure. Thats a good idea.

On Mon, May 9, 2016 at 7:46 PM, Davor Cubranic [email protected]
wrote:

I was under the impression that use was just BEGIN { require thing; import
thing; } What makes require more fault tolerant?

I can include it in an eval {} block and set a status variable that's
checked in Rserve::access to make
sure the package is imported before calling it.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#214 (comment)

@cubranic
Copy link
Contributor Author

cubranic commented May 9, 2016

Also, since you have used Moose, you may be able to answer this. Is it
dangerous to include it in the safe compartment. Its big and complicated
and does dynamic method creation, but unless it allows users to eval code
or access files its probably ok.

I'm just a Moose user, so I can't really answer that. FWIW, it's the first OO package mentioned by perlootut and is probably the de-facto/future standard Perl's OO framework. If that's really a no-go, I can look into replacing it with one of the others mentioned in perlootut, although (as you might imagine), I'd love not to have to do that at this point.

@goehle
Copy link
Member

goehle commented May 10, 2016

You aren't really using a lot of the fancier Moose functionality and you
dont have any inheritance or subclassing going on. This afternoon I was
able to get a working version of Statistics::R::IO::Rserve running without
Moose at all. (Basically you just have to manually define the constructor
and all of the accessors.) If you were to do anything, I would remove
Moose altogether. Doing so would make the package a lot lighter and may
improve load times (since this stuff is getting compiled every time a
student views a problem or hits submit). However, its not really that big
a deal and Moose is probably fine.

On Mon, May 9, 2016 at 7:51 PM, Davor Cubranic [email protected]
wrote:

Also, since you have used Moose, you may be able to answer this. Is it
dangerous to include it in the safe compartment. Its big and complicated
and does dynamic method creation, but unless it allows users to eval code
or access files its probably ok.

I'm just a Moose user, so I can't really answer that. FWIW, it's the first
OO package mentioned by perlootut and is probably the de-facto/future
standard Perl's OO framework. If that's really a no-go, I can look into
replacing it with one of the others mentioned in perlootut, although (as
you might imagine), I'd love not to have to do that at this point.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#214 (comment)

@mgage
Copy link
Member

mgage commented May 10, 2016

Hi Goeff,

Did you use Class::Accessor ? I used that in attempts table to streamline
the constructor/accessor/setter code. It does the job without all the infrastructure
required by Moose.

I even added the “antlers” option as an option since
that impliments moose-like syntax, but I ended up not using it and I believe we’re removing it
because it caused some trouble with installation on Danny’s system.

Take care,

mike

On May 9, 2016, at 8:08 PM, Geoff Goehle [email protected] wrote:

You aren't really using a lot of the fancier Moose functionality and you
dont have any inheritance or subclassing going on. This afternoon I was
able to get a working version of Statistics::R::IO::Rserve running without
Moose at all. (Basically you just have to manually define the constructor
and all of the accessors.) If you were to do anything, I would remove
Moose altogether. Doing so would make the package a lot lighter and may
improve load times (since this stuff is getting compiled every time a
student views a problem or hits submit). However, its not really that big
a deal and Moose is probably fine.

On Mon, May 9, 2016 at 7:51 PM, Davor Cubranic [email protected]
wrote:

Also, since you have used Moose, you may be able to answer this. Is it
dangerous to include it in the safe compartment. Its big and complicated
and does dynamic method creation, but unless it allows users to eval code
or access files its probably ok.

I'm just a Moose user, so I can't really answer that. FWIW, it's the first
OO package mentioned by perlootut and is probably the de-facto/future
standard Perl's OO framework. If that's really a no-go, I can look into
replacing it with one of the others mentioned in perlootut, although (as
you might imagine), I'd love not to have to do that at this point.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#214 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_pg_pull_214-23issuecomment-2D218025696&d=CwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=WJKYhUlwnbS-m3hft9tSeTp0n5Mz8RC1yBpxzDyw5eo&s=REKF9dz8KcJfF0m8LyAre_FqmvUpZj1G8UK64qJy--U&e=

@cubranic
Copy link
Contributor Author

Hm, Class::Accessor doesn't have destructors, which I use for closing the connection to Rserve. (Which may be done by Perl too during GC, but I'm not sure about it.) Following up on perlootut, I looked at Class::Tiny and saw that by using Class::Tiny::Antlers, I'd get a near-Moose syntax so hopefully significantly reducing my work to switch to it. And all this with no non-core dependencies!

I hope there isn't a catch in the fine print that I missed. I'll have to give this a try and will report back with my findings.

@cubranic
Copy link
Contributor Author

I created a very simple class hierarchy using Class::Tiny::Antlers that captures pretty much everything I used in Moose and tested out using this from a Webwork macro similar to RserveClient.pl. It worked once I added Class::Tiny to the safe compartment (the line in defaults.conf is [qw(Rserve Class::Tiny)]. So if this would be an acceptable solution to you, I'll proceed to convert Statistics::R::IO to use this OO framework and then update this PR.

@mgage, when you used Class::Accessor, did you need to add it to the safe compartment? I was mildly disappointed to see that Class::Tiny was still required.

@goehle
Copy link
Member

goehle commented May 22, 2016

That solution works for me. Thanks for going the extra mile and slimming
the code down a bit.

P.G. Mike used Class::Accessor on the WeBWorK side, so Class::Accessor is
not in the safe cmpt.

Geoff.

On Sun, May 22, 2016 at 7:13 PM, Davor Cubranic [email protected]
wrote:

I created a very simple class hierarchy using Class::Tiny::Antlers that
captures pretty much everything I used in Moose and tested out using this
from a Webwork macro similar to RserveClient.pl. It worked once I added
Class::Tiny to the safe compartment (the line in defaults.conf is [qw(Rserve
Class::Tiny)]. So if this would be an acceptable solution to you, I'll
proceed to convert Statistics::R::IO to use this OO framework and then
update this PR.

@mgage https://github.com/mgage, when you used Class::Accessor, did you
need to add it to the safe compartment? I was mildly disappointed to see
that Class::Tiny was still required.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#214 (comment)

@cubranic
Copy link
Contributor Author

Just a quick update: I converted Statistics::R::IO to use Class::Accessor and it looks good so far. All of my tests pass and the test WebWork problem still works correctly. I still need to release it on CPAN and then just need to clean up the code in this pull request before it's ready.

cubranic added a commit to cubranic/Statistics-R-IO that referenced this pull request May 25, 2016
See openwebwork/pg#214 for the full discussion, but the gist is that
project is using such a small slice of Moose that it could just as
easily be replaced with Class::Tiny.
cubranic added a commit to cubranic/Statistics-R-IO that referenced this pull request May 26, 2016
See openwebwork/pg#214, specifically the comment by @goehle on
9-May-2016: this should allow leaving IO::File out of PG's safe
compartment and doesn't seem to have any other negative side-effect.
@cubranic
Copy link
Contributor Author

I released a new version of Statistics::R::IO that uses Class::Tiny and it is available through CPAN (v1.0001). So this should now work.

The line in wwk2/defaults.conf for adding the required modules to the safe compartment now looks like this:

[qw(Rserve Class::Tiny IO::Handle)],

If you can test it and give me OK from your end, I'll rebase onto current develop and squish into a single commit, and then submit a separate pull request on webwork2/develop for the configuration change.

@goehle
Copy link
Member

goehle commented May 30, 2016

I tested this and it worked with a sample problem. I also tested it on a server that didn't have Rserve or R::IO::Statistics and it didn't complain when running regular problems. Looks good to me. Thanks for going the extra mile.

@cubranic
Copy link
Contributor Author

OK, I cleaned up the commits and opened the PR for the config file: openwebwork/webwork2#700.

@goehle
Copy link
Member

goehle commented May 31, 2016

This works for me on a couple of problems and doesn't cause issues when its not configured on non rserve problems.

@goehle goehle merged commit 29a4a8a into openwebwork:develop May 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants