Merge branch 'main' of https://github.com/natcap/invest into bugfix/1…

…645-gdal-exceptions-in-validation

Conflicts:
	requirements.txt

HISTORY.rst

@@ -51,18 +51,20 @@ Unreleased Changes
       https://github.com/natcap/invest/issues/1293
 * Workbench
     * Several small updates to the model input form UI to improve usability
-      and visual consistency (https://github.com/natcap/invest/issues/912)
+      and visual consistency (https://github.com/natcap/invest/issues/912).
     * Fixed a bug that caused the application to crash when attempting to
       open a workspace without a valid logfile
-      (https://github.com/natcap/invest/issues/1598)
+      (https://github.com/natcap/invest/issues/1598).
     * Fixed a bug that was allowing readonly workspace directories on Windows
-      (https://github.com/natcap/invest/issues/1599)
+      (https://github.com/natcap/invest/issues/1599).
     * Fixed a bug that, in certain scenarios, caused a datastack to be saved
       with relative paths when the Relative Paths checkbox was left unchecked
-      (https://github.com/natcap/invest/issues/1609)
+      (https://github.com/natcap/invest/issues/1609).
+    * Improved error handling when a datastack cannot be saved with relative
+      paths across drives (https://github.com/natcap/invest/issues/1608).
 * Habitat Quality
-    * Access raster is now generated from the reprojected access vector.
-      (https://github.com/natcap/invest/issues/1615)
+    * Access raster is now generated from the reprojected access vector
+      (https://github.com/natcap/invest/issues/1615).
 * Urban Flood Risk
     * Fields present on the input AOI vector are now retained in the output.
       (https://github.com/natcap/invest/issues/1600)

src/natcap/invest/datastack.py

@@ -535,6 +535,9 @@ def build_parameter_set(args, model_name, paramset_path, relative=False):
 
     Returns:
         ``None``
+
+    Raises:
+        ValueError if creating a relative path fails.
     """
     def _recurse(args_param):
         if isinstance(args_param, dict):
@@ -552,8 +555,16 @@ def _recurse(args_param):
                     if (normalized_path == '.' or
                             os.path.dirname(paramset_path) == normalized_path):
                         return '.'
-                    temp_rel_path = os.path.relpath(
-                        normalized_path, os.path.dirname(paramset_path))
+                    try:
+                        temp_rel_path = os.path.relpath(
+                            normalized_path, os.path.dirname(paramset_path))
+                    except ValueError:
+                        # On Windows, ValueError is raised when ``path`` and
+                        # ``start`` are on different drives
+                        raise ValueError(
+                            """Error: Cannot save datastack with relative
+                            paths across drives. Choose a different save
+                            location, or use absolute paths.""")
                     # Always save unix paths.
                     linux_style_path = temp_rel_path.replace('\\', '/')
                 else:

src/natcap/invest/ui_server.py

@@ -175,17 +175,29 @@ def write_parameter_set_file():
         relativePaths: boolean
 
     Returns:
-        A string.
+        A dictionary with the following key/value pairs:
+        - message (string): for logging and/or rendering in the UI.
+        - error (boolean): True if an error occurred, otherwise False.
     """
     payload = request.get_json()
     filepath = payload['filepath']
     modulename = payload['moduleName']
     args = json.loads(payload['args'])
     relative_paths = payload['relativePaths']
 
-    datastack.build_parameter_set(
-        args, modulename, filepath, relative=relative_paths)
-    return 'parameter set saved'
+    try:
+        datastack.build_parameter_set(
+            args, modulename, filepath, relative=relative_paths)
+    except ValueError as message:
+        LOGGER.error(str(message))
+        return {
+            'message': str(message),
+            'error': True
+        }
+    return {
+        'message': 'Parameter set saved',
+        'error': False
+    }
 
 
 @app.route(f'/{PREFIX}/save_to_python', methods=['POST'])
@@ -221,15 +233,26 @@ def build_datastack_archive():
         args: JSON string of InVEST model args keys and values
 
     Returns:
-        A string.
+        A dictionary with the following key/value pairs:
+        - message (string): for logging and/or rendering in the UI.
+        - error (boolean): True if an error occurred, otherwise False.
     """
     payload = request.get_json()
-    datastack.build_datastack_archive(
-        json.loads(payload['args']),
-        payload['moduleName'],
-        payload['filepath'])
-
-    return 'datastack archive created'
+    try:
+        datastack.build_datastack_archive(
+            json.loads(payload['args']),
+            payload['moduleName'],
+            payload['filepath'])
+    except ValueError as message:
+        LOGGER.error(str(message))
+        return {
+            'message': str(message),
+            'error': True
+        }
+    return {
+        'message': 'Datastack archive created',
+        'error': False
+    }
 
 
 @app.route(f'/{PREFIX}/log_model_start', methods=['POST'])
@@ -251,6 +274,7 @@ def log_model_exit():
         payload['status'])
     return 'OK'
 
+
 @app.route(f'/{PREFIX}/languages', methods=['GET'])
 def get_supported_languages():
     """Return a mapping of supported languages to their display names."""

tests/test_datastack.py

@@ -6,10 +6,10 @@
 import pprint
 import shutil
 import sys
-import tarfile
 import tempfile
 import textwrap
 import unittest
+from unittest.mock import patch
 
 import numpy
 import pandas
@@ -397,6 +397,25 @@ def test_archive_extraction(self):
             os.path.join(spatial_csv_dir, spatial_csv_dict[4]['path']),
             target_csv_vector_path)
 
+    def test_relative_path_failure(self):
+        """Datastack: raise error when relative path creation fails."""
+        from natcap.invest import datastack
+        params = {
+            'workspace_dir': os.path.join(self.workspace),
+        }
+
+        archive_path = os.path.join(self.workspace, 'archive.invs.tar.gz')
+
+        # Call build_datastack_archive and force build_parameter_set
+        # to raise an error
+        error_message = 'Error saving datastack'
+        with self.assertRaises(ValueError):
+            with patch('natcap.invest.datastack.build_parameter_set',
+                       side_effect=ValueError(error_message)):
+                datastack.build_datastack_archive(
+                    params, 'test_datastack_modules.simple_parameters',
+                    archive_path)
+
 
 class ParameterSetTest(unittest.TestCase):
     """Test Datastack."""
@@ -506,6 +525,26 @@ def test_relative_parameter_set(self):
         self.assertEqual(invest_version, __version__)
         self.assertEqual(callable_name, modelname)
 
+    def test_relative_path_failure(self):
+        """Datastack: raise error when relative path creation fails."""
+        from natcap.invest import datastack
+
+        params = {
+            'data_dir': os.path.join(self.workspace, 'data_dir'),
+        }
+        modelname = 'natcap.invest.foo'
+        paramset_filename = os.path.join(self.workspace, 'paramset.json')
+
+        # make the sample data so filepaths are interpreted correctly
+        os.makedirs(params['data_dir'])
+
+        # Call build_parameter_set and force it into an error state
+        with self.assertRaises(ValueError):
+            with patch('natcap.invest.os.path.relpath',
+                       side_effect=ValueError):
+                datastack.build_parameter_set(
+                    params, modelname, paramset_filename, relative=True)
+
     @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows")
     def test_relative_parameter_set_windows(self):
         """Datastack: test relative parameter set paths saved linux style."""

tests/test_ui_server.py

@@ -122,14 +122,41 @@ def test_write_parameter_set_file(self):
             }),
             'relativePaths': True,
         }
-        _ = test_client.post(
+        response = test_client.post(
             f'{ROUTE_PREFIX}/write_parameter_set_file', json=payload)
+        self.assertEqual(
+            response.json,
+            {'message': 'Parameter set saved', 'error': False})
         with open(filepath, 'r') as file:
             actual_data = json.loads(file.read())
         self.assertEqual(
             set(actual_data),
             {'args', 'invest_version', 'model_name'})
 
+    def test_write_parameter_set_file_error_handling(self):
+        """UI server: write_parameter_set_file endpoint
+        should catch a ValueError and return an error message.
+        """
+        test_client = ui_server.app.test_client()
+        self.workspace_dir = tempfile.mkdtemp()
+        filepath = os.path.join(self.workspace_dir, 'datastack.json')
+        payload = {
+            'filepath': filepath,
+            'moduleName': 'natcap.invest.carbon',
+            'args': json.dumps({
+                'workspace_dir': 'foo'
+            }),
+            'relativePaths': True,
+        }
+        error_message = 'Error saving datastack'
+        with patch('natcap.invest.datastack.build_parameter_set',
+                   side_effect=ValueError(error_message)):
+            response = test_client.post(
+                f'{ROUTE_PREFIX}/write_parameter_set_file', json=payload)
+            self.assertEqual(
+                response.json,
+                {'message': error_message, 'error': True})
+
     def test_save_to_python(self):
         """UI server: save_to_python endpoint."""
         test_client = ui_server.app.test_client()
@@ -163,11 +190,42 @@ def test_build_datastack_archive(self):
                 'carbon_pools_path': data_path
             }),
         }
-        _ = test_client.post(
+        response = test_client.post(
             f'{ROUTE_PREFIX}/build_datastack_archive', json=payload)
+        self.assertEqual(
+            response.json,
+            {'message': 'Datastack archive created', 'error': False})
         # test_datastack.py asserts the actual archiving functionality
         self.assertTrue(os.path.exists(target_filepath))
 
+    def test_build_datastack_archive_error_handling(self):
+        """UI server: build_datastack_archive endpoint
+        should catch a ValueError and return an error message.
+        """
+        test_client = ui_server.app.test_client()
+        self.workspace_dir = tempfile.mkdtemp()
+        target_filepath = os.path.join(self.workspace_dir, 'data.tgz')
+        data_path = os.path.join(self.workspace_dir, 'data.csv')
+        with open(data_path, 'w') as file:
+            file.write('hello')
+
+        payload = {
+            'filepath': target_filepath,
+            'moduleName': 'natcap.invest.carbon',
+            'args': json.dumps({
+                'workspace_dir': 'foo',
+                'carbon_pools_path': data_path
+            }),
+        }
+        error_message = 'Error saving datastack'
+        with patch('natcap.invest.datastack.build_datastack_archive',
+                   side_effect=ValueError(error_message)):
+            response = test_client.post(
+                f'{ROUTE_PREFIX}/build_datastack_archive', json=payload)
+            self.assertEqual(
+                response.json,
+                {'message': error_message, 'error': True})
+
     @patch('natcap.invest.ui_server.usage.requests.post')
     @patch('natcap.invest.ui_server.usage.requests.get')
     def test_log_model_start(self, mock_get, mock_post):

workbench/src/renderer/components/SaveAsModal/index.jsx

@@ -67,6 +67,7 @@ class SaveAsModal extends React.Component {
   }
 
   handleShow() {
+    this.props.removeSaveErrors();
     this.setState({
       relativePaths: false,
       show: true,

workbench/src/renderer/components/SetupTab/index.jsx

@@ -100,6 +100,7 @@ class SetupTab extends React.Component {
     this.savePythonScript = this.savePythonScript.bind(this);
     this.saveJsonFile = this.saveJsonFile.bind(this);
     this.setSaveAlert = this.setSaveAlert.bind(this);
+    this.removeSaveErrors = this.removeSaveErrors.bind(this);
     this.wrapInvestExecute = this.wrapInvestExecute.bind(this);
     this.investValidate = this.investValidate.bind(this);
     this.debouncedValidate = this.debouncedValidate.bind(this);
@@ -234,8 +235,8 @@ class SetupTab extends React.Component {
       relativePaths: relativePaths,
       args: JSON.stringify(args),
     };
-    const response = await writeParametersToFile(payload);
-    this.setSaveAlert(response);
+    const {message, error} = await writeParametersToFile(payload);
+    this.setSaveAlert(message, error);
   }
 
   async saveDatastack(datastackPath) {
@@ -249,14 +250,16 @@ class SetupTab extends React.Component {
       args: JSON.stringify(args),
     };
     const key = window.crypto.getRandomValues(new Uint16Array(1))[0].toString();
-    this.setSaveAlert('archiving...', key);
-    const response = await archiveDatastack(payload);
-    this.setSaveAlert(response, key);
+    this.setSaveAlert('archiving...', false, key);
+    const {message, error} = await archiveDatastack(payload);
+    this.setSaveAlert(message, error, key);
   }
 
   /** State updater for alert messages from various save buttons.
    *
    * @param {string} message - the message to display
+   * @param {boolean} error - true if message was generated by an error,
+   *        false otherwise. Defaults to false.
    * @param {string} key - a key to uniquely identify each save action,
    *        passed as prop to `Expire` so that it can be aware of whether to,
    *        1. display: because a new save occurred, or
@@ -267,10 +270,28 @@ class SetupTab extends React.Component {
    */
   setSaveAlert(
     message,
+    error = false,
     key = window.crypto.getRandomValues(new Uint16Array(1))[0].toString()
   ) {
     this.setState({
-      saveAlerts: { ...this.state.saveAlerts, ...{ [key]: message } }
+      saveAlerts: {
+        ...this.state.saveAlerts,
+        ...{ [key]: {
+          message,
+          error
+      }}}
+    });
+  }
+
+  removeSaveErrors() {
+    const alerts = this.state.saveAlerts;
+    Object.keys(alerts).forEach((key) => {
+      if (alerts[key].error) {
+        delete alerts[key];
+      }
+    });
+    this.setState({
+      saveAlerts: alerts
     });
   }
 
@@ -490,18 +511,22 @@ class SetupTab extends React.Component {
 
       const SaveAlerts = [];
       Object.keys(saveAlerts).forEach((key) => {
-        const message = saveAlerts[key];
+        const { message, error } = saveAlerts[key];
         if (message) {
-          // Alert won't expire during archiving; will expire 2s after completion
-          const alertExpires = (message === 'archiving...') ? 1e7 : 2000;
+          // Alert won't expire during archiving; will expire 4s after completion
+          // Alert won't expire when an error has occurred;
+          // will be hidden next time save modal opens
+          const alertExpires = (error || message === 'archiving...') ? 1e7 : 4000;
           SaveAlerts.push(
             <Expire
               key={key}
               className="d-inline"
               delay={alertExpires}
             >
-              <Alert variant="success">
-                {message}
+              <Alert
+                variant={error ? 'danger' : 'success'}
+              >
+                {t(message)}
               </Alert>
             </Expire>
           );
@@ -564,6 +589,7 @@ class SetupTab extends React.Component {
               savePythonScript={this.savePythonScript}
               saveJsonFile={this.saveJsonFile}
               saveDatastack={this.saveDatastack}
+              removeSaveErrors={this.removeSaveErrors}
             />
             <React.Fragment>
               {SaveAlerts}