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

Requirements for mlbackend_python tests missing #9

Open
andrewnicols opened this issue Aug 24, 2017 · 6 comments
Open

Requirements for mlbackend_python tests missing #9

andrewnicols opened this issue Aug 24, 2017 · 6 comments

Comments

@andrewnicols
Copy link
Contributor

It'd be great to include the python library required to run the mlbackend_python tests.

There were 11 skipped tests:

1) core_analytics_prediction_testcase::test_ml_training_and_prediction with data set "no_splitting-\mlbackend_python\processor" ('\core\analytics\time_splittin...itting', 0, '\mlbackend_python\processor')
Skipping \mlbackend_python\processor as the predictor is not ready.

/var/www/html/analytics/tests/prediction_test.php:141
/var/www/html/lib/phpunit/classes/advanced_testcase.php:80
@danpoltawski
Copy link
Contributor

See https://github.com/danpoltawski/moodle-docker/issues/29 ideally, I don't think thus should be part of this image as it seems a better fit for a dedicated container.

caperneoignis referenced this issue in caperneoignis/moodle-php-apache Dec 29, 2017
Related issues: danpoltawski/docker-moodle/#9.
caperneoignis referenced this issue in caperneoignis/moodle-php-apache Dec 29, 2017
Related issues: danpoltawski/docker-moodle/#9.
caperneoignis referenced this issue in caperneoignis/moodle-php-apache Dec 29, 2017
Related issues: danpoltawski/docker-moodle/#9.
@dmonllao
Copy link
Contributor

dmonllao commented Dec 3, 2018

Hi, I've been looking at this for some time now. I've set up a new image with the python package installed. It can be run from the local filesystem setting $CFG->pathtopython to docker run --rm -ti dmonllao/moodle-mlbackend:test python (dmonllao/moodle-mlbackend:test is just this: https://github.com/dmonllao/moodle-docker-mlbackend) but the docker command is not accessible from the webserver container and I imagine that installing docker on webserver is not nice. Our mlbackend does not have any REST API nor something like that and it is just a CLI command executed from the webserver (changing this is a separate topic and it should part of the roadmap now that I have time for analytics stuff) but, in the meanwhile, I don't have any other proposal than to install python and the moodlemlbackend pip package in https://github.com/moodlehq/moodle-php-apache/blob/master/Dockerfile although I haven't tried if php:7.1-apache-stretch comes with apt-get. I am 100% open to suggestions.

@dmonllao
Copy link
Contributor

dmonllao commented Dec 10, 2018

I think that, it is not worth spending more time in the new-container approach until the python ml backend does not allow execution from a remote server. I've modified both moodle-php-apache and moodle-docker so we can execute python mlbackend tests using docker:
dmonllao@python-mlbackend
dmonllao/moodle-docker@python-mlbackend

I didn't set $CFG->pathtopython into if (getenv('MOODLE_DOCKER_PHPUNIT_EXTRAS')) because we set PHPUNIT_LONGTEST to true and because python dependencies will be installed anyway in moodle-php-apache.

The patch has been tested in my local machine. I built new moodle-php-apache tags and I updated moodle-docker's base.yml to point to my local image and tag. Analytics tests that involve using the python backend to generated predictions passed without skips (although including an annoying and useless warning from one of the moodlemlbackend package dependencies)

➜ moodle-docker git:(master) ✗ bin/moodle-docker-compose exec webserver vendor/bin/phpunit analytics/tests/prediction_test.php
Moodle 3.7dev (Build: 20181205), f8b5fcd304bd1621ec475003b9b6c546c01f4c3f
Php: 7.1.24, pgsql: 9.6.7, OS: Linux 4.4.0-139-generic x86_64
PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

.../usr/local/lib/python2.7/dist-packages/matplotlib/font_manager.py:273: UserWarning: Matplotlib is building the font cache using fc-list. This may take a moment.
warnings.warn('Matplotlib is building the font cache using fc-list. This may take a moment.')
................ 19 / 19 (100%)

Time: 1.26 minutes, Memory: 66.00MB

OK (19 tests, 189 assertions)

@stronk7
Copy link
Member

stronk7 commented Dec 10, 2018

+1 to create a PR about this issue!

@dmonllao
Copy link
Contributor

Thanks, I've created moodlehq/moodle-docker#90 and #35

@dmonllao
Copy link
Contributor

Hi, the patches above install the python ml backend in the web server. Moodle will be able to communicate with the python backend once https://tracker.moodle.org/browse/MDL-66004 is installed. I set up an interim docker image for it in https://hub.docker.com/r/dmonllao/moodle-mlbackend-python. It would be great if someone from iteam could confirm that the proposed approach is enough to satisfy the requirements to deploy it in iteam's infrastructure.

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

No branches or pull requests

4 participants