Skip to content

Moving configuration and files to PG#651

Closed
pstaabp wants to merge 15 commits into
openwebwork:developfrom
pstaabp:move-config-to-pg
Closed

Moving configuration and files to PG#651
pstaabp wants to merge 15 commits into
openwebwork:developfrom
pstaabp:move-config-to-pg

Conversation

@pstaabp

@pstaabp pstaabp commented Feb 5, 2022

Copy link
Copy Markdown
Member

This is a draft PR to get a discussion going plus some help. Here's some big picture ideas about this

  • expansion of PGEnvironment module to 1) read the conf/pg_defaults.yml file 2) process much of it (say to build up nested directories)
  • move pg-related configuration from defaults.config (on ww2 side) into conf/pg_defaults.yml
  • addition of a standaloneRenderer code (in bin) from https://github.com/mgage/standaloneProblemRenderer and adaption of it to only use modules from pg-side.
  • To get standaloneRenderer working, need to move many modules from webwork side to pg side.
  • All of the modules on pg side should be independent of webwork. The main thing needed is to replace CourseEnvironment with PGEnvironment and update the path to get configurations.

A few things I'm thinking about/having trouble with probably because of my lack of knowledge of the way pg works in detail:

  • I think I have got Localization compiling on pg-side (though not tested). I copied all of the .po files from ww side. I think only the pg side things need to be there.
  • I commented out some things about the Delayed Mailer. It seems like that shouldn't be on the pg-side. Can we get this back to webwork2.
  • In FormatRenderedProblem (now on pg side), there are a number of calls to $ce->{...} which don't seem to exist in the code. Is this automatically added by LTI ?
  • In FormatRenderedProblem, there is information about the theme? Is this really needed?
  • Perhaps we need two different FormatRenderedProblem code (one on each side) or a wrapper on the webwork side?

To play with this so far, running bin/standaloneRenderer.pl on a problem with various flags. It doesn't finish, but runs a lot of code.

This clearly isn't done and once we can get the standalone renderer working, need to remove modules from webwork2 side.

Please let me know if there is something I have clearly not done well. Also, I think there might be other modules needed to be moved to PG.

@pstaabp

pstaabp commented Feb 17, 2022

Copy link
Copy Markdown
Member Author

A new draft. First, in an attempt to clean up and clarify packages, there is now a lib/Renderer directory where I'm hoping to keep all packages related to rendering a problem. This touches a lot, but I think helps separate in our minds what is PG.

Although not currently working (hence a draft PR), the meat of this is in Renderer::Problem. You create a problem passing in a problem source (not a file path), and then render the problem. The idea will be to get the results as a hash of everything we typically expect back. Currently, this isn't working. There's some issues with the safe compartment, but am pushing this up to get some feedback.

I have a very simple script bin/test_render_problem.pl that loads in a problem from the OPL and attempts to set it up to render. Currently I'm getting an error:

Can't locate object method "current" via package "Parser::Context" (perhaps you forgot to load "Parser::Context"?) at /opt/webwork/pg/macros/PG.pl line 16.

Comment thread htdocs/package.json Outdated
"prepare": "npm run generate-css"
},
"dependencies": {
"bootstrap": "^5.1.3",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added that before my latest round of changes. I'm trying to strip out as much html/css/js as possible.

Comment thread lib/WWSafe.pm Outdated
}

use Data::Dumper;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Comment thread lib/Renderer/Translator.pm Outdated
Comment on lines +225 to +226
print "in Translator::new\n";
# print Dumper $safe_cmpt;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print "in Translator::new\n";
# print Dumper $safe_cmpt;

Comment thread lib/Renderer/Translator.pm Outdated
Comment on lines +167 to +168
print Dumper "in evaluate_modules";
print Dumper \@modules;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print Dumper "in evaluate_modules";
print Dumper \@modules;

my ( $module, @extra_packages ) = @$module_packages_ref;

my ($module, @extra_packages) = @$module_packages_ref;
warn "Loading $module\n";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
warn "Loading $module\n";

#use PadWalker; # used for processing error messages
#use Data::Dumper;

use Data::Dumper;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use Data::Dumper;

Comment thread lib/Renderer/Problem.pm
use PGEnvironment;
use Renderer::Translator;
use Renderer::Localize;
use Data::Dumper;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use Data::Dumper;

Comment thread lib/Renderer/Problem.pm
return shift, 0; # no errors
}

sub defineProblemEnvir {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't all this duplicating PG_env?

Comment thread lib/Renderer/IO.pm
/^1\./ and $mod = "WeBWorK::PG::IO::WW1";
/^2\./ and $mod = "WeBWorK::PG::IO::WW2";
/^Daemon\s*2\./ and $mod = "WeBWorK::PG::IO::Daemon2";
/^1\./ and $mod = "Renderer::IO::WW1";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't WW1 being deleted in this PR?

Comment thread lib/Renderer/Localize.pm Outdated
use strict;
use warnings;

use feature "say";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use feature "say";

Comment thread lib/Renderer/Localize.pm Outdated
Comment on lines +20 to +21
use Data::Dumper;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use Data::Dumper;

@drdrew42

drdrew42 commented Feb 18, 2022

Copy link
Copy Markdown
Member
  • I commented out some things about the Delayed Mailer. It seems like that shouldn't be on the pg-side. Can we get this back to webwork2.

I only see DelayedMailer used in PG::Local, and there it is created and then added to the problem environment as an extra. The defineProblemEnvir subroutine adds the DelayedMailer as {mailer} which does not appear anywhere else AFAICT.

  • In FormatRenderedProblem (now on pg side), there are a number of calls to $ce->{...} which don't seem to exist in the code. Is this automatically added by LTI ?

FormatRenderedProblem::formatRenderedProblem L#82-6 defines a new course environment as $ce?
Edit: It looks like you've replaced CourseEnvironment with PG environment... so isn't this expected?

  • In FormatRenderedProblem, there is information about the theme? Is this really needed?
    Perhaps we need two different FormatRenderedProblem code (one on each side) or a wrapper on the webwork side?

It looks like theme information in FormatRenderedProblem is only used for the footer information. Seems safe to remove.

@drdrew42 drdrew42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the source of your errors -- you're loading PG.pl too soon, try loading the other perl_libraries first.

Comment thread lib/Renderer/Problem.pm
Comment on lines +92 to +107
############################################################################
# evaluate modules and "extra packages"
############################################################################

#warn "PG: evaluating modules and \"extra packages\"\n";

for my $module_packages_ref (@{ $self->{pg_env}->{perl_modules} }) {
my ($module, @extra_packages) = @$module_packages_ref;

# the first item is the main package
$self->{translator}->evaluate_modules($module);

# the remaining items are "extra" packages
$self->{translator}->load_extra_packages(@extra_packages);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should come right after Translator->new, then we need to set the translator's environment: $translator->environment($self->{pg_env}); (this used to be set with the results of defineProblemEnvir, which I'm assuming is supplanted by the new pg_environment object), and then after all that -- we can load in PG.pl with unrestricted_load...

@mgage mgage left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the guts of the AttemptsTable should be left on the callers (e.g. webwork2) side of the renderer -- it reports the status of the graded problem to the student and the styling and method of presentation should be up to the caller. (e.g. the instead of an html answer table the calling program might like to use some fancier framework, including using overlays of the answer blanks when they are moused over -- to indicate which answers are right and what is wrong with the other answers. The information for this is passed back from process_answers( a subroutine in Translator.pm) through rh_evaluated_answers entry in the pg object. Eventually this information is returned to the caller through, I believe, %PG_ANSWERS_HASH which is part of the output of ENDDOCUMENT in PG.pl. The information is also in the PG object which is passed back although currently we use the contents of the %PG_ANSWER_HASH to report information. AttemptsTable subroutines use this data to communicate the information to the student. That might be different for different use cases so AttemptsTable should not be part of the render itself.

@mgage

mgage commented Jun 27, 2022

Copy link
Copy Markdown
Member

I suggest concentrating on getting the configuration files (and whatever reads those files working) so that we can make the configuration of webwork2 and pg as separated as possible (hopefully completely separated). It should be possible to plug these two configurations back in to an existing webwork2 course and test the concept. Drawing the configurations from two different files should not upset the current operation of webwork2 and pg and we can test whether we've left some configurations out, or are duplicating some of them or if there are hidden inter-dependencies which we have overlooked

@pstaabp

pstaabp commented Oct 5, 2022

Copy link
Copy Markdown
Member Author

This PR will be superseded by #709 and closed once that is merged.

@pstaabp

pstaabp commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Closed in favor of #709 and #726

@pstaabp pstaabp closed this Oct 12, 2022
@pstaabp pstaabp deleted the move-config-to-pg branch December 9, 2022 11:23
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.

4 participants