giellalt / giella-core

Build tools and build support files as well as developer support tools for the GiellaLT repositories.
https://giellalt.uit.no
GNU General Public License v3.0
7 stars 2 forks source link

Start using GiellaLTGramTools #44

Closed albbas closed 6 months ago

albbas commented 8 months ago

GiellaLTGramTools gather the scripts gramcheck-test.py, gramcheck_comparator.py and make_grammarchecker_zip.py into a separate, pipx installable repo.

albbas commented 8 months ago

If --enable-gramcheck is enabled in lang-xxx, we need the check if divvun-checker exists in the path and if GiellaLTGramTools is installed

flammie commented 8 months ago

if I got it right we want somethign like this in template-lang-und:

diff --git a/m4/giella-macros.m4 b/m4/giella-macros.m4
index f740727..7c0508e 100644
--- a/m4/giella-macros.m4
+++ b/m4/giella-macros.m4
@@ -165,16 +165,20 @@ AM_CONDITIONAL([CAN_YAML_TEST], [test "x$enable_yamltests" != xno])
 ################ LXML or pip ################
 AS_IF([test "x$enable_grammarchecker" != "xno"],
      [AM_PATH_PYTHON([3.5],, [:])
-     AX_PYTHON_MODULE(lxml)
-     AX_PYTHON_MODULE(pip)
-     AC_MSG_CHECKING([whether we can use lxml])
-     AS_IF([test "x$HAVE_PYMOD_LXML" != "xyes"],
-           AS_IF([test "x$HAVE_PYMOD_PIP" != "xno"],
-                 AC_MSG_RESULT(no)
-                 AC_MSG_WARN([lxml or pip is needed for grammarcheckers]),
-                 AC_MSG_RESULT([no but using pip])),
-           AC_MSG_RESULT(yes))])
-
+     AX_PYTHON_MODULE(pipx)
+     AC_PATH_PROG([MAKE_GRAMMARCHECKER_ZIP], [make_grammarchecker_zip], [no])
+     AC_PATH_PROG([GRAMCHECK_YAML], [gramcheck-yaml], [no])
+     AS_IF([test "x$MAKE_GRAMMARCHECKER_ZIP" = xno],
+           [AS_IF([test "x$HAVE_PYMOD_PIPX" = xno],
+                 [AC_MSG_ERROR([make_gramchecker_zip is needed for --enable grammarchecker. 
+                               please python -m pip install pipx and then 
+                               python -m pipx install git+https://github.com/giellalt/giellaltgram])],
+                 [AC_MSG_ERROR([make_gramchecer_zip is needed for --enable-grammarchecker. 
+                  please python -m pipx install git+https://github.com/giellalt/giellaltgram])]
+            )]
+        )
+     ])
+    
 AM_CONDITIONAL([CAN_LXML], [test "x$HAVE_PYMOD_LXML" != xno])
 AM_CONDITIONAL([CAN_PIP], [test "x$HAVE_PYMOD_LXML" != xno])
 ################ Generated documentation ################
albbas commented 8 months ago

Yes, that seems right.

About the installation of pipx, it might be best to point to the official documetation: https://pipx.pypa.io/stable/installation/ We also need a check to see if divvun-checker is present, as gramcheck-yaml and gramcheck-xml use it to run the tests

albbas commented 8 months ago

So something like this:

AC_PATH_PROG([PIPX], [pipx], [no])
AC_PATH_PROG([MAKE_GRAMMARCHECKER_ZIP], [make_grammarchecker_zip], [no])
AC_PATH_PROG([DIVVUN_CHECKER], [divvun-checker], [no])
albbas commented 8 months ago

And we will need to set up installation of giellaltgramtools in taskcluster

albbas commented 8 months ago

Something akin to this in taskcluster-gha:

diff --git a/lang/install-deps/index.ts b/lang/install-deps/index.ts
index 7122d53..779ef7e 100644
--- a/lang/install-deps/index.ts
+++ b/lang/install-deps/index.ts
@@ -33,7 +33,7 @@ async function run() {
         "bc"
     ]

-    const devPackages = ["foma", "hfst", "libhfst-dev", "cg3-dev", "divvun-gramcheck", "python3-corpustools", "python3-lxml", "python3-yaml"]
+    const devPackages = ["foma", "hfst", "libhfst-dev", "cg3-dev", "divvun-gramcheck", "pipx", "python3-yaml"]

     if (requiresApertium) {
         devPackages.push("apertium")
@@ -47,6 +47,7 @@ async function run() {
     await ProjectJJ.addNightlyToApt(requiresSudo)
     await Apt.install(devPackages, requiresSudo)
     await Ssh.cleanKnownHosts()
+    await Pipx.install("https://github.com/giellalt/GiellaLTGramTools") // Pipx is an imagined class
 }

 run().catch(err => {
albbas commented 6 months ago

The gramcheck thingy has been moved here: https://github.com/divvun/giellaltgramtools

When installed, it provides gtgramtool

gramtool provides three subcommands:

gtgramtool build-archive replaces make_grammarchecker_zip gtgramtool test yaml replaces gramcheck-test.py gtgramtool test xml replaces gramcheck_comparator.py

We should make the change, python 3.12 really doesn't like to install dependencies into global library directories.

snomos commented 6 months ago

@flammie could you do this:

  1. review the code
  2. resolve conflicts in giella-core before merging
  3. update all lang repos to require the new version of giella-core
  4. test that the lang repos work as they should with the new scripts, including installation using pipx
  5. update the taskcluster scripts as well as the GitHub action scripts
  6. push all changes in this order:
    1. giella-core
    2. taskcluster + GHA repos
    3. lang repos
  7. check that the builds go through as they should
snomos commented 6 months ago

Regarding the help text when pipx is not found: the present text did not work for me. What did work was:

brew install pipx
pipx install git+https://github.com/divvun/giellaltgramtools
snomos commented 6 months ago

When building the SMJ grammar checker using this branch it fails with the following error:

echo lxml and pip is missing so this may fail...:
lxml and pip is missing so this may fail...:
gtgramtool build-archive pipespec.xml smj.zcheck
make[3]: gtgramtool: No such file or directory

@albbas do you see what is wrong?

albbas commented 6 months ago

From your description above it seems that you haven't ran pipx ensurepath after brew install pipx. This command makes sure that pipx-installed commands are added to the path, that may be the reason that gtgramtool is not found.

As for the messages about pip and lxml: we don't need an explicit check for them anymore, as they are hidden away and automatically installed when using pipx.

I'll remove that check from am-shared/tools-grammarcheckers-dir-include.am

snomos commented 6 months ago

@albbas: the build is presently failing with this error:

to make target 'scripts/make_grammarchecker_zip.py', needed by 'all-am'.  Stop.
albbas commented 6 months ago

Using version 0.2.0 of GiellaLTGramTools, the use_gramtools branch of giella-core and giella-macros.m4 from the above mentioned template-lang-und PR building and testing grammarcheckers work in lang-sme, -sma, -smj and -smn