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

Start using GiellaLTGramTools #44

Merged
merged 7 commits into from
Apr 12, 2024
Merged

Start using GiellaLTGramTools #44

merged 7 commits into from
Apr 12, 2024

Conversation

albbas
Copy link
Contributor

@albbas albbas commented Feb 21, 2024

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

@albbas
Copy link
Contributor Author

albbas commented Feb 21, 2024

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
Copy link
Contributor

flammie commented Feb 21, 2024

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
Copy link
Contributor Author

albbas commented Feb 21, 2024

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
Copy link
Contributor Author

albbas commented Feb 21, 2024

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
Copy link
Contributor Author

albbas commented Feb 21, 2024

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

@albbas
Copy link
Contributor Author

albbas commented Feb 21, 2024

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
Copy link
Contributor Author

albbas commented Apr 4, 2024

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
Copy link
Member

snomos commented Apr 5, 2024

@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
Copy link
Member

snomos commented Apr 5, 2024

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
Copy link
Member

snomos commented Apr 5, 2024

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
Copy link
Contributor Author

albbas commented Apr 5, 2024

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
Copy link
Member

snomos commented Apr 6, 2024

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

to make target 'scripts/make_grammarchecker_zip.py', needed by 'all-am'.  Stop.

@albbas
Copy link
Contributor Author

albbas commented Apr 6, 2024

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

@flammie flammie marked this pull request as ready for review April 11, 2024 21:14
@flammie flammie merged commit 9b5b845 into main Apr 12, 2024
1 check passed
@flammie flammie deleted the use_gramtools branch April 15, 2024 10:26
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.

3 participants