From 1eb13f2629bed5405e3570e314086de0eb94c468 Mon Sep 17 00:00:00 2001 From: JP Date: Wed, 12 Feb 2020 00:42:15 +1100 Subject: [PATCH] Message callback (#612) * rejig loadPackage message callbacks * adjust to keep tests working * console log the resolve message * test loading after failure * appease linter --- docs/api_reference.md | 11 ++--- src/pyodide.js | 87 ++++++++++++++++++++++++------------ src/runpython.c | 5 ++- test/test_package_loading.py | 23 ++++++++-- 4 files changed, 87 insertions(+), 39 deletions(-) diff --git a/docs/api_reference.md b/docs/api_reference.md index 397e388a2..e33df02be 100644 --- a/docs/api_reference.md +++ b/docs/api_reference.md @@ -60,7 +60,7 @@ The object as nested Python lists. ## Javascript API -### pyodide.loadPackage(names) +### pyodide.loadPackage(names, messageCallback, errorCallback) Load a package or a list of packages over the network. @@ -73,6 +73,7 @@ The package needs to be imported from Python before it can be used. |-------------------|-----------------|---------------------------------------| | *names* | {String, Array} | package name, or URL. Can be either a single element, or an array. | | *messageCallback* | function | A callback, called with progress messages. (optional) | +| *errorCallback* | function | A callback, called with error/warning messages. (optional) | *Returns* @@ -81,7 +82,7 @@ Loading is asynchronous, therefore, this returns a `Promise`. ### pyodide.loadedPackages -`Array` with loaded packages. +`Object` with loaded packages. Use `Object.keys(pyodide.loadedPackages)` to access the names of the loaded packages, and `pyodide.loadedPackages[package_name]` to access @@ -160,7 +161,7 @@ Runs a string of code. The last part of the string may be an expression, in whic | *jsresult* | *any* | Result, converted to Javascript | -### pyodide.runPythonAsync(code, messageCallback) +### pyodide.runPythonAsync(code, messageCallback, errorCallback) Runs Python code, possibly asynchronously loading any known packages that the code chunk imports. @@ -187,8 +188,8 @@ pyodide.runPythonAsync(code, messageCallback) | name | type | description | |-------------------|----------|--------------------------------| | *code* | String | Python code to evaluate | -| *messageCallback* | function | Callback given status messages | -| | | (optional) | +| *messageCallback* | function | A callback, called with progress messages. (optional) | +| *errorCallback* | function | A callback, called with error/warning messages. (optional) | *Returns* diff --git a/src/pyodide.js b/src/pyodide.js index f4399f2f4..ecde23db1 100644 --- a/src/pyodide.js +++ b/src/pyodide.js @@ -11,7 +11,7 @@ var languagePluginLoader = new Promise((resolve, reject) => { //////////////////////////////////////////////////////////// // Package loading - let loadedPackages = new Array(); + let loadedPackages = {}; var loadPackagePromise = new Promise((resolve) => resolve()); // Regexp for validating package name and URI var package_name_regexp = '[a-z0-9_][a-z0-9_\-]*' @@ -93,19 +93,34 @@ var languagePluginLoader = new Promise((resolve, reject) => { } } - let _loadPackage = (names, messageCallback) => { + let _loadPackage = (names, messageCallback, errorCallback) => { + if (messageCallback == undefined) { + messageCallback = () => {}; + } + if (errorCallback == undefined) { + errorCallback = () => {}; + } + let _messageCallback = (msg) => { + console.log(msg); + messageCallback(msg); + }; + let _errorCallback = (errMsg) => { + console.error(errMsg); + errorCallback(errMsg); + }; + // DFS to find all dependencies of the requested packages let packages = self.pyodide._module.packages.dependencies; let loadedPackages = self.pyodide.loadedPackages; let queue = [].concat(names || []); - let toLoad = new Array(); + let toLoad = {}; while (queue.length) { let package_uri = queue.pop(); const pkg = _uri_to_package_name(package_uri); if (pkg == null) { - console.error(`Invalid package name or URI '${package_uri}'`); + _errorCallback(`Invalid package name or URI '${package_uri}'`); return; } else if (pkg == package_uri) { package_uri = 'default channel'; @@ -113,20 +128,23 @@ var languagePluginLoader = new Promise((resolve, reject) => { if (pkg in loadedPackages) { if (package_uri != loadedPackages[pkg]) { - console.error(`URI mismatch, attempting to load package ` + - `${pkg} from ${package_uri} while it is already ` + - `loaded from ${loadedPackages[pkg]}!`); + _errorCallback(`URI mismatch, attempting to load package ` + + `${pkg} from ${package_uri} while it is already ` + + `loaded from ${loadedPackages[pkg]}!`); return; + } else { + _messageCallback(`${pkg} already loaded from ${loadedPackages[pkg]}`) } } else if (pkg in toLoad) { if (package_uri != toLoad[pkg]) { - console.error(`URI mismatch, attempting to load package ` + - `${pkg} from ${package_uri} while it is already ` + - `being loaded from ${toLoad[pkg]}!`); + _errorCallback(`URI mismatch, attempting to load package ` + + `${pkg} from ${package_uri} while it is already ` + + `being loaded from ${toLoad[pkg]}!`); return; } } else { - console.log(`Loading ${pkg} from ${package_uri}`); + console.log( + `${pkg} to be loaded from ${package_uri}`); // debug level info. toLoad[pkg] = package_uri; if (packages.hasOwnProperty(pkg)) { @@ -136,7 +154,7 @@ var languagePluginLoader = new Promise((resolve, reject) => { } }); } else { - console.error(`Unknown package '${pkg}'`); + _errorCallback(`Unknown package '${pkg}'`); } } } @@ -159,10 +177,8 @@ var languagePluginLoader = new Promise((resolve, reject) => { return; } - const packageList = Array.from(Object.keys(toLoad)).join(', '); - if (messageCallback !== undefined) { - messageCallback(`Loading ${packageList}`); - } + let packageList = Array.from(Object.keys(toLoad)); + _messageCallback(`Loading ${packageList.join(', ')}`) // monitorRunDependencies is called at the beginning and the end of each // package being loaded. We know we are done when it has been called @@ -177,10 +193,22 @@ var languagePluginLoader = new Promise((resolve, reject) => { } delete self.pyodide._module.monitorRunDependencies; self.removeEventListener('error', windowErrorHandler); - if (!isFirefox) { - preloadWasm().then(() => {resolve(`Loaded ${packageList}`)}); + + let resolveMsg = `Loaded `; + if (packageList.length > 0) { + resolveMsg += packageList.join(', '); } else { - resolve(`Loaded ${packageList}`); + resolveMsg += 'no packages' + } + + if (!isFirefox) { + preloadWasm().then(() => { + console.log(resolveMsg); + resolve(resolveMsg); + }); + } else { + console.log(resolveMsg); + resolve(resolveMsg); } } }; @@ -204,14 +232,17 @@ var languagePluginLoader = new Promise((resolve, reject) => { } else { scriptSrc = `${package_uri}`; } + _messageCallback(`Loading ${pkg} from ${scriptSrc}`) loadScript(scriptSrc, () => {}, () => { // If the package_uri fails to load, call monitorRunDependencies twice // (so packageCounter will still hit 0 and finish loading), and remove - // the package from toLoad so we don't mark it as loaded. - console.error(`Couldn't load package from URL ${scriptSrc}`) - let index = toLoad.indexOf(pkg); - if (index !== -1) { - toLoad.splice(index, 1); + // the package from toLoad so we don't mark it as loaded, and remove + // the package from packageList so we don't say that it was loaded. + _errorCallback(`Couldn't load package from URL ${scriptSrc}`); + delete toLoad[pkg]; + let packageListIndex = packageList.indexOf(pkg); + if (packageListIndex !== -1) { + packageList.splice(packageListIndex, 1); } for (let i = 0; i < 2; i++) { self.pyodide._module.monitorRunDependencies(); @@ -229,11 +260,11 @@ var languagePluginLoader = new Promise((resolve, reject) => { return promise; }; - let loadPackage = (names, messageCallback) => { + let loadPackage = (names, messageCallback, errorCallback) => { /* We want to make sure that only one loadPackage invocation runs at any * given time, so this creates a "chain" of promises. */ - loadPackagePromise = - loadPackagePromise.then(() => _loadPackage(names, messageCallback)); + loadPackagePromise = loadPackagePromise.then( + () => _loadPackage(names, messageCallback, errorCallback)); return loadPackagePromise; }; @@ -374,7 +405,7 @@ var languagePluginLoader = new Promise((resolve, reject) => { // filesystem to install itself. Once that's complete, it will be replaced // by the call to `makePublicAPI` with a more limited public API. self.pyodide = pyodide(Module); - self.pyodide.loadedPackages = new Array(); + self.pyodide.loadedPackages = {}; self.pyodide.loadPackage = loadPackage; }, () => {}); }, () => {}); diff --git a/src/runpython.c b/src/runpython.c index 0fddcafb9..e236d0814 100644 --- a/src/runpython.c +++ b/src/runpython.c @@ -69,7 +69,7 @@ EM_JS(int, runpython_init_js, (), { return Module._runPythonInternal(pycode); }; - Module.runPythonAsync = function(code, messageCallback) + Module.runPythonAsync = function(code, messageCallback, errorCallback) { var pycode = allocate(intArrayFromString(code), 'i8', ALLOC_NORMAL); @@ -100,7 +100,8 @@ EM_JS(int, runpython_init_js, (), { } if (Object.keys(packages).length) { var runInternal = function() { return new Promise(internal); }; - return Module.loadPackage(Object.keys(packages), messageCallback) + return Module + .loadPackage(Object.keys(packages), messageCallback, errorCallback) .then(runInternal); } } diff --git a/test/test_package_loading.py b/test/test_package_loading.py index 89520a340..efa7dd5a6 100644 --- a/test/test_package_loading.py +++ b/test/test_package_loading.py @@ -82,8 +82,8 @@ def test_load_packages_multiple(selenium_standalone, packages): # The log must show that each package is loaded exactly once, # including when one package is a dependency of the other # ('pyparsing' and 'matplotlib') - assert selenium.logs.count(f'Loading {packages[0]}') == 1 - assert selenium.logs.count(f'Loading {packages[1]}') == 1 + assert selenium.logs.count(f'Loading {packages[0]} from') == 1 + assert selenium.logs.count(f'Loading {packages[1]} from') == 1 @pytest.mark.parametrize('packages', [['pyparsing', 'pytz'], @@ -104,8 +104,8 @@ def test_load_packages_sequential(selenium_standalone, packages): # The log must show that each package is loaded exactly once, # including when one package is a dependency of the other # ('pyparsing' and 'matplotlib') - assert selenium.logs.count(f'Loading {packages[0]}') == 1 - assert selenium.logs.count(f'Loading {packages[1]}') == 1 + assert selenium.logs.count(f'Loading {packages[0]} from') == 1 + assert selenium.logs.count(f'Loading {packages[1]} from') == 1 def test_different_ABI(selenium_standalone): @@ -154,6 +154,21 @@ def test_load_handle_failure(selenium_standalone): assert 'Loading pyparsing' in selenium.logs # <- this fails +def test_load_failure_retry(selenium_standalone): + """Check that a package can be loaded after failing to load previously""" + selenium = selenium_standalone + selenium.load_package('http://invalidurl/pytz.js') + assert selenium.logs.count('Loading pytz from') == 1 + assert selenium.logs.count("Couldn't load package from URL") == 1 + assert selenium.run_js('return Object.keys(pyodide.loadedPackages)') == [] + + selenium.load_package('pytz') + selenium.run('import pytz') + assert selenium.logs.count('Loading pytz from') == 2 + assert selenium.run_js( + 'return Object.keys(pyodide.loadedPackages)') == ['pytz'] + + def test_load_package_unknown(selenium_standalone): url = selenium_standalone.server_hostname port = selenium_standalone.server_port