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

WeBWorK 2.19 support (Tacoma 2024 project) #2336

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Alex-Jordan
Copy link
Contributor

This is a large PR, just in time for everyone to be going on vacation :)

I'm doing my best to group the contributions from @drgrice1. @drdrew42, and myself from Tacoma. According to my testing, with these changes:

  1. Nothing significantly changes (using the minimal webwork example and the sample-chapter) when you use a WeBWorK 2.17 or 2.18 server, like the AIM webwork-ptx server or the Runestone book support server. It would be good if we work out a way to test that Runestone books aren't broken if this is merged and makes its way to the CLI.
  2. Things work (and work better) when there is a WeBWorK 2.19 server involved. The AIM webwork-dev server is using 2.19.

Once using a 2.19 server, there are new features that I have tested and documented using the minimal webwork example and the sample-chapter.

  • You can use local .pg files in the external folder.
  • You can do the static processing of PG using a local instance of the pg repository. For me in testing today, this took close to 1/4 the time of using the AIM webwork-dev server. You do need to install that pg repo is all.
  • You can do the static processing of PG using an instance of the standalone renderer that is also located on webwork-dev. For me in testing today, this took close to 80% of the time of using the AIM webwork-dev server.

Possibly, we need a schedule for pulling the trigger on this. Here's a draft.

  1. Rob reviews.
  2. Alex amends according to Rob's review
  3. Work with @bnmnetp to check that RS books are not adversely affected.
  4. Rob merges.
  5. Work with @bnmnetp to also check that RS books are not adversely affected if the RS support server moves to version 2.19.
  6. Alex upgrades both the RS support server and AIM's webwork-ptx to version 2.19.

@drgrice1
Copy link
Contributor

Thanks for putting this together for us.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

I've checked all of this against existing behavior, and will have some comments in a minute.

But, I don't see any reason to not start testing behavior on Runestone now, step (3) of above with @bnmnetp.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

Thanks, @Alex-Jordan for putting all of this together. Some comments:

  • Are you sure you want to load LaTeX macros into an attribute (@data-macros)? Could they not be accessed from an element? Looks mildly dangerous to me.
  • I think "choice" type problems are losing their \item in the conversion to LaTeX lists. Sample Chapter, Checkpoint 1.6.1 about sqrt(2) being rational or not, is a good first example.
  • Using the makefile to generate problem sets seems to land them someplace much deeper than expected in the output. Might just be a problem with the makefile, I haven't investigated too deep.

Not much, given the quantity of code. Good job. I'm going to familiarize myself with the new features now, so may have more to say. But wanted to get this going in the right direction as quickly as possible.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

Using the makefile to generate problem sets seems to land them someplace much deeper

OK, I see rendering-data in a file now, I was confused by the diff I was looking at. Scratch that comment.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

And with AIM's 2.19 server, the \item in the LaTeX conversion are back, so this may just be an issue for backward compatibility.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

  • Built with AIM dev server in publication file. Problems hang with "Loading" when I click Activate. Firefox and Chrome.
  • Guide has two sections "Using a local copy of PG", which need to be edited/merged?

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

@static-processing="local"

Can't locate Mojo/Base.pm in @INC (you may need to install the Mojo::Base module) (@INC entries checked: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.38.2 /usr/local/share/perl/5.38.2 /usr/lib/x86_64-linux-gnu/perl5/5.38 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.38 /usr/share/perl/5.38 /usr/local/lib/site_perl) at /home/rob/mathbook/mathbook/script/webwork/pg-ptx.pl line 3.
BEGIN failed--compilation aborted at /home/rob/mathbook/mathbook/script/webwork/pg-ptx.pl line 3.

Ideally, an Ubuntu package would provide what I need?

https://packages.ubuntu.com/oracular/libmojolicious-perl

@drgrice1
Copy link
Contributor

Yes, the libmojolicious-perl package provides what is needed. That is the primary down side of the local approach. It does require that the necessary Perl modules be installed.

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

Thanks, @drgrice1! I needed the following Ubuntu packages to get through the sample chapter.

libmojolicious-perl
liblocale-maketext-lexicon-perl
libgd-perl
libdbi-perl
libuuid-tiny-perl
libclass-tiny-perl

@rbeezer
Copy link
Collaborator

rbeezer commented Dec 26, 2024

Built representations throug to the end. Some non-fatal errors. About 6 instances, problem numbers 62-68 (or so), and maybe the very last one. Can't tell what I might be missing.

PTX:INFO    : sending webwork-62 to socket to save in /home/rob/mathbook/mathbook/examples/webwork/sample-chapter/generated/webwork/webwork-representations.xml: origin is 'generated'
errors:
ERRORS from evaluating PG file:
Undefined subroutine &main::latexImagePreamble called at line 7 of 
warnings:
Can't locate macro file |WWSC.pl| via path: |.|,<br/> |[PG]/macros/|,<br/> |[PG]/macros//answers|,<br/> |[PG]/macros//capa|,<br/> |[PG]/macros//contexts|,<br/> |[PG]/macros//core|,<br/> |[PG]/macros//deprecated|,<br/> |[PG]/macros//graph|,<br/> |[PG]/macros//math|,<br/> |[PG]/macros//misc|,<br/> |[PG]/macros//parsers|,<br/> |[PG]/macros//ui|
PTX:ERROR: WeBWorK problem generated/webwork/pg/Integrating_WeBWorK_into_Textbooks/1_8_1-A_static_lateximage_graph.pg with seed 62 is either empty or failed to compile  
  Use -a to halt with full PG and returned content

@Alex-Jordan
Copy link
Contributor Author

Thanks Rob! Some quick responses and I may do more this evening.

Are you sure you want to load LaTeX macros into an attribute (@data-macros)? Could they not be accessed from an element? Looks mildly dangerous to me.

What's going on here is the PTX HTML page has a div with id latex-macros. The contents of that div will be the LaTeX macros, but wrapped in \(...\). Now, later the webwork js needs to get the latex macros to write them into the srcdoc for an iframe where the WW problem will render. So how will it get them? I have it accessing them from where they are stored redundantly as a data attribute on the div#latex-macros (without the \(...\)). It's been a while, but I think I felt uneasy with having it access them from the content of the div#latex-macros. Particularly because the \(...\) would already be present. Possibly it could change to access them from the content and remove the redundant data attribute. If there is a future need to add macros at this stage, either I'd have to peel off the \(...\) or revert to how it is now.

I think "choice" type problems are losing their \item in the conversion to LaTeX lists. Sample Chapter, Checkpoint 1.6.1 about sqrt(2) being rational or not, is a good first example. And with AIM's 2.19 server, the \item in the LaTeX conversion are back, so this may just be an issue for backward compatibility.

OK, I will follow up with this.

Using the makefile to generate problem sets seems to land them someplace much deeper than expected in the output. Might just be a problem with the makefile, I haven't investigated too deep.

I neglected to test with the makefile but I'll do that to make sure things are still working. I was testing by running pretext/pretext/pretext directly. But actually this sounds like an expected change (that I forgot to mention in all the mix of things). Prior to this PR, when you make an archive of PG files, that gets made wherever you happen to be. Or possibly you can specify a -d or a -o. But now it all gets placed in an appropriate subfolder of the generated folder. I think this is a good change.

Built with AIM dev server in publication file. Problems hang with "Loading" when I click Activate. Firefox and Chrome.

Are you viewing with file://? Same thing happened to me if I didn't view through an actual web server.

Guide has two sections "Using a local copy of PG", which need to be edited/merged?

Sounds like an oversight. Will follow up.

Ideally, an Ubuntu package would provide what I need?

I'll add to the documentation as best I can about what packages are needed. Or maybe a generic "you may need to install some perl packages". With Ubuntu packages, using cpanm, and more variants, I'm not sure about getting too specific.

Undefined subroutine &main::latexImagePreamble

This indicates a host course doesn't have the access it needs to the .pl file for WWSC. I'll look into it.

@drgrice1
Copy link
Contributor

Undefined subroutine &main::latexImagePreamble

This indicates a host course doesn't have the access it needs to the .pl file for WWSC. I'll look into it.

I think that this was when @rbeezer was testing the local approach based on the output line "sending webwork-62 to socket". The local approach adds the macros directory in the generated directory to the macro path. So the macro should be available. Although, I also don't see that path in the output that @rbeezer showed.

@Alex-Jordan
Copy link
Contributor Author

I think "choice" type problems are losing their \item

Looking over the diff, I think this is from -assembly where I replaced var[@form] with ul, ol, dl as appropriate. I think if I bring back support for var[@form] as equivalent to ul[@form], that will address the backwards compatibility.

@Alex-Jordan
Copy link
Contributor Author

I think "choice" type problems are losing their \item

OK, this one is addressed. I added a commit (rather than force push) but it can be squashed later as it indicates.

@Alex-Jordan
Copy link
Contributor Author

Guide has two sections "Using a local copy of PG", which need to be edited/merged?

Copy-paste oversight. The second one should have had its title changed to be about using a renderer.

@Alex-Jordan
Copy link
Contributor Author

@drgrice1 Do you have a recommendation about what I should put in the documentation about which perl version (or later) is required for local PG processing?

@drgrice1
Copy link
Contributor

I am not exactly sure how old of a Perl version will work for local PG processing. I know that Perl 5.16 or newer is certainly needed. I suspect that versions that old will have problems though. I think that 5.26 is probably the oldest that we should try to support, and so I guess put that in the documentation. Although you would need 5.32 or newer for a version that I can truly guarantee everything will work for.

@Alex-Jordan
Copy link
Contributor Author

I just pushed an additional commit with some guidance for the perl version and packages. I think what remains is:

  • the @data-macros comments...does anything need to change?
  • problems hanging in the live rendering (was the page being viewed from file://?)
  • what was going on with Undefined subroutine &main::latexImagePreamble? Maybe need to meet live at a drop-in to look into that?
  • testing a Runestone build won't break. Will follow up on an email thread with @bnmnetp and @rbeezer

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 1, 2025

Publication file: webwork/@static-processing="local"

Use the Sample Chapter stanza for sample-chapter-macros target.

Now have fresh (within PTX distribution):

./examples/webwork/generated/webwork/pg/Integrating_WeBWorK_into_Textbooks/macros/WWSC.pl

but when doing sample-chapter-representations I still get

PTX:INFO    : sending webwork-62 to socket to save in /home/rob/mathbook/mathbook/examples/webwork/sample-chapter/generated/webwork/webwork-representations.xml: origin is 'generated'
errors:
ERRORS from evaluating PG file:
Undefined subroutine &main::latexImagePreamble called at line 7 of 
warnings:
Can't locate macro file |WWSC.pl| via path: |.|,<br/> |[PG]/macros/|,<br/> |[PG]/macros//answers|,<br/> |[PG]/macros//capa|,<br/> |[PG]/macros//contexts|,<br/> |[PG]/macros//core|,<br/> |[PG]/macros//deprecated|,<br/> |[PG]/macros//graph|,<br/> |[PG]/macros//math|,<br/> |[PG]/macros//misc|,<br/> |[PG]/macros//parsers|,<br/> |[PG]/macros//ui|
PTX:ERROR: WeBWorK problem generated/webwork/pg/Integrating_WeBWorK_into_Textbooks/1_8_1-A_static_lateximage_graph.pg with seed 62 is either empty or failed to compile  
  Use -a to halt with full PG and returned content

Is that book-title directory in the way? I'm not 100% sure what I'm debugging here exactly. And other PreTeXt projects (that other people are hanging fire on) are going to keep me busy.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 1, 2025

I may merge #2341 in a bit. Which will require a rebase here. (And perhaps you can fix in (not squash) the newer commits along the way?)

@Alex-Jordan
Copy link
Contributor Author

OK, as suspected, I was not hitting that error because at some previous time I had manually placed the needed macro file in an OK place. But it's working now with my testing. And while fixing this up, I noticed parallel issues with making a problem set archive and fixed those too.

Summary:

  • When you use pretext-ww-problem-sets.xsl or pretext-pg-macros.xsl directly (which we don't do), it's always going to create a folder named like Integrating_WeBWorK_into_Textbooks in the working directory. This will have subfolders with problems, def files, header files, and macros if we used pretext-ww-problem-sets.xsl. If we only used pretext-pg-macros.xsl, it will only have like Integrating_WeBWorK_into_Textbooks/macros/WWSC.pl.
  • When you invoke these via pretext/pretext/pretext (or indirectly with the Makefile), if there is no generated folder, then the "destination directory" takes the place of the working directory.
  • When you invoke these via pretext/pretext/pretext, if there is a generated folder, then the "destination directory" will effectively be generated/webwork/pg.

So for example if you run make sample-chapter-macros, then look in sample-chapter/generated/webwork/pg/Integrating_WeBWorK_into_Textbooks/macros/ to see the .pl file. And if you are then building representations locally, you don't need to do anything; it knows to look there for the macros.

Lastly, say you clear out your generated folder, and then run make sample-chapter-representations with local processing. Actually one of the first things the corresponding python routine does is make the macros. So that's enough; no need to make the macros first. It's only when the processing will be done by a webwork2 server that we need to build macros ahead of time and manually place them in the webwork2 server.

@Alex-Jordan
Copy link
Contributor Author

We tested at RSA and for a while we had some issues with multiple choice questions. But we've ironed that all out. So I think this is ready unless there is more PTX-side testing to do. After this is merged, my next step would be to upgrade the AIM WW server (the production one) to version 2.19. Of course, with advanced warning posted on -announce. (We don't expect bad consequences, but it's always possible.)

Eventually the changes here would make it into the CLI. Then we could upgrade the RSA WW server to 2.19, but I think it is less stressful to wait and do that in June.

The commits here are still separated in case it helps to only put eyes on new changes since whenever the last review was. Let me know if you would prefer I (a) squash all related commits [for example, all that are about the js] or (b) squash it all into one big commit. Ideally we somehow still assign credit (and blame) to me, Drew, and Glenn though. git wants one single email address for each commit, even when it is muddy which bits were mine, which were Drew's, and which were Glenn's. So I mostly used my email address for these commits, and therefore GH wants to say these commits are mine. But where it matters, I put multiple names (like "Jordan, Parker, Rice") as the commit author's name. One option is to squash down to three: one all me, one that is all Glenn, and one that is "Jordan, Parker, Rice".

@Alex-Jordan
Copy link
Contributor Author

Oh, I also rebased from master since there was a small conflict.

@rbeezer
Copy link
Collaborator

rbeezer commented Jan 17, 2025

Thanks - I'll try to address this one in about a week. If there is no change for status quo (as is the intent) then I'm inclined to merge this with new features being subject to change after used in the wild. If you haven't already, you might prepare a blurb for -announce. Thanks.

@Alex-Jordan
Copy link
Contributor Author

I force pushed a small change regarding the recent thread on -dev about showPartialCorrectAnswers. It was only a relatively small change to pretext-webwork.js and only makes a difference on problems where showPartialCorrectAnswers = 0.

@Alex-Jordan
Copy link
Contributor Author

They do not belong in generated (which is likely in my .gitignore), nor do I think that location should be moving as part of this PR.

I'm not changing this. It is necessary for how local PG processing is done now.

@rbeezer
Copy link
Collaborator

rbeezer commented Feb 26, 2025

OK, maybe I made a mistake in testing, I thought.

No changes at all, except current "master" versus this PR on top of master.

Nothing is being output for the problem sets with the PR in the location where they are being output with master.

Would you please confirm and if so, diagnose?

@Alex-Jordan
Copy link
Contributor Author

Alex-Jordan commented Feb 27, 2025 via email

@bnmnetp
Copy link
Contributor

bnmnetp commented Feb 27, 2025

Alex,

I think I hear you, BUT...

This will almost always cause conflicts for an automated build where the generated folder has changed from last weeks build.

Things that are generated by the code in the repository should NOT be committed! If you have a repository then you should be able to reliably recreate Version A of whatever with revision B of pretext without fail.

Github is not a cache!!

Especially when using the CLI, it is fine to rebuild everything and then let the CLI handle changes.

@rbeezer
Copy link
Collaborator

rbeezer commented Feb 27, 2025

As I said, pg files, set definitions, etc now must go inside generated.

One use case is you have a book with a bunch of WW problems authored in PreTeXt.. A stylesheet makes a collection of problems that you can cart over to some WW server of your choice and use. No more pretext, no more Runestone - a standalone collection of WW problems. And the collection can be created wherever you want it to be. That is no longer functioning and needs to be restored.

Whatever else new you might need for local rendering is another discussion, and existing behavior needs to be preserved first.

@Alex-Jordan
Copy link
Contributor Author

Alex-Jordan commented Feb 27, 2025 via email

@rbeezer
Copy link
Collaborator

rbeezer commented Feb 27, 2025

The pretext/pretext script has a feature invoked with -f webwork-sets.

With this PR, that feature is not working as expected. So that needs to be addressed before this can move forward. Makefile says that a -z switch will make a tarball.

@Alex-Jordan
Copy link
Contributor Author

Just force pushed. This follows a rebase off master.

  • Trailing slashes in sample chapter restored.
  • Changed radio buttons in PDF again. Now to \bigcirc. I believe these are the best simple option for unchecked radio buttons that a reader might pencil in.
  • Removed @data-macros. Note that I cannot access the LaTeX macros from the div where they are written for regular PTX HTML. Because by the time I would need to read them with javascript, MathJax has already manipulated that content into something unrecognizable. So I added the macros redundantly within that div as text within a p (all hidden, of course). If you think of something better, let me know.
  • Restore pretext -c all -f webwork-sets ability to specify a destination (so you can still see all that output from the sample chapter where you are expecting it to still be). Accomplish this by letting None be a valid destination, in which case the destination will actually be generated/webwork/pg (where I need it to be for other things).

Sorry for deviating from commit message scheme, but I figure you will squash this all anyway. It's hard to manage the contributions from three people here in a way consistent with your commit message scheme.

@Alex-Jordan
Copy link
Contributor Author

Just putting out a light ping about this. I recognize there were several dozen open PRs recently, and a lot being done. But also I started to worry that I did not highlight enough how I think the issue about PG set archiving should be OK now. At least, that is my expectation.

@rbeezer
Copy link
Collaborator

rbeezer commented Mar 17, 2025

Still working on changes to webwork_sets().

# make PG macros that may be needed
pg_macros(xml_source, pub_file, stringparams, dest_dir)

Do the macros need to be distinct from the remainder? Building these early means the construction of a *.tgz in the temporary directory will not pick these up. Is that intentional?

if pub_file:
    stringparams["publisher"] = pub_file
    generated_dir, _ = get_managed_directories(xml_source, pub_file)
else:
    generated_dir = None

get_managed_directories() should always do the right thing. And so should not be error-checked after a call. If it is not doing the right thing, then it should be fixed on another PR. I see this at least one ohter place.

if dest_dir is None:
    if not (os.path.isdir(os.path.join(generated_dir, 'webwork', 'pg'))):
        os.makedirs(os.path.join(generated_dir, 'webwork', 'pg'))
    dest_dir = os.path.join(generated_dir, 'webwork', 'pg')

Similarly, the destination directory should default to the current working directory, at least. Before it gets passed into any libary method. Is that not happening?

# With multiple files, we need to copy a tree
# copy_build_directory() doesn't work for this and seems like a bug
# Could replace copytree() with copy_build_directory() once bug is fixed
shutil.copytree(folder, os.path.join(dest_dir, folder_name), dirs_exist_ok=True)

seems like a bug

How so? shutil.copytree() is known to be buggy. While we have used copy_build_directory() for a while without complaints?

@Alex-Jordan
Copy link
Contributor Author

Do the macros need to be distinct from the remainder? Building these early means the construction of a *.tgz in the temporary directory will not pick these up. Is that intentional?

A few things to know:

  • There can be a need to build (or rebuild) the macro library file, and you don't need to rebuild everything else. So this is demonstrated in the Makefile.
  • The underlying python function is pg_macros. And for making a full archive of pg files, set def files, etc, the underlying python function is webwork_sets. And webwork_sets calls pg_macros. So if your goal is to make a cull archive, it will include the macro libraries file.

And so should not be error-checked after a call.

OK, I wasn't aware get_managed_directories returns None when no publisher file is given. If I understand right I can run generated_dir, _ = get_managed_directories(xml_source, pub_file) outside that if block and remove the else block. By "error checking", do you mean where I was initializing generated_dir to None? Want to make sure I am not missing something about your comment.

Similarly, the destination directory should default to the current working directory, at least.

This is part of the dance that I came up with to continue letting the archive be built in the destination directory but also letting it (when needed) be built within the generated folder.

I think today in one of the group threads, there was mention of WW using base64 encoding. With this PR, base64 is gone (as long as the WW server is 2.19 and above). This is not needed because the .pg files themselves are put in a predictable place within the generated folder. So there is a general need to produce the problem files (the .pg files) and know ahead of time (using managed directories) where they are. But also for backwards compatibility of make sample-chapter-problem-sets, to produce them in the destination folder.

To make both possible, if the dest_dir argument to the webwork_sets function is explicitly None, that is the signal to build the archive folder tree within the generated folder. A user of pretext/pretext/pretext cannot directly pass None as that argument. It's only when webwork_sets is called by webwork_to_xml that None is used for the "destination directory".

Forcing me to explain myself in writing, I see now that this abuse of None could probably be cleared away. Instead I could have webwork_to_xml pass the generated folder as the destination folder argument to webwork_sets. I'll do that when I next make edits.

seems like a bug

Too long ago to remember but when I get into it again I will check. Although I do not presently remember details, I do remember this was failing to copy over the full tree. Foggy memory notwithstanding, I think it was failing to copy the folder tree in the situations where I need it to copy the folder tree to within the generated folder.

@rbeezer
Copy link
Collaborator

rbeezer commented Mar 17, 2025

So if your goal is to make a cull archive, it will include the macro libraries file.

The *.tgz version of the archive is made from contents of the temporary directory. It looks to me like the change writes the macros to the destination before compressing/consolidating.

Want to make sure I am not missing something about your comment.

I think you've got it. Getting the managed directories should always return something sensible. I think that might be None in some cases (never specified an "external" directory). Similarly, the destination directory should always be sensible for calls from the library. So vaious checks and changes should not be necessary. But experiment, since I cannot be trusted. Add some print statments and try a few things.

there was mention of WW using base64 encoding.

thread about STACK. You could mention this there.

Instead I could have webwork_to_xml pass the generated folder as the destination folder argument to webwork_sets

That sounds exactly like how things should work.

Too long ago to remember but when I get into it again I will check.

Maybe the macros went missing. ;-)

Short version: webwork_sets() has been working fine. You can certainly use it to make the PG you need someplace (anyplace) over in "generated" -- that would be the best way to get all that. But any direct changes to webwork_sets() likely do not belong on this PR and need a good standalone argument for their necessity.

@bnmnetp
Copy link
Contributor

bnmnetp commented Mar 22, 2025

I would love to see this get closed, so that we can restore full function to active calculus and others. I'm happy to try to help but the conversation above is pretty far into the weeds as far as my familiarity with the code.

If there is something I can do to help ensure this gets done soon, please let me know.

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