Repository: intake/intake

Issue:
Unbound local error when setting cache path with environment variables
Hi there,
I've been experiencing an error when providing intake with a local cache path using environment variables.
I'm using python 3.12.8 and intake 2.0.7

## Steps to reproduce

- set an environment variable (using bash on linux)

```bash
export INTAKE_CACHE="icache"
```
- start a python console and import intake

I would expect intake to store files in a folder called `icache` in the current working directory. However, I get this error:
>   File "/home/jroettenbacher/miniforge3/envs/update-requirements/lib/python3.12/site-packages/intake/config.py", line 162, in load_env
    self[k2] = val
               ^^^
UnboundLocalError: cannot access local variable 'val' where it is not associated with a value

As far as I can tell the problem arises as `ast.literal_eval()`, which loads the environment variables, does not like string input. Except it wants everything to be quoted even strings. So `'"icache"'` would work and would create a folder called `"icache"`, which I don't want. Linux can do that but Windows throws an error because of this.

The [load_env()](https://github.com/intake/intake/blob/69c6565e8bed77a61f7e71e697053cea9ca5fb4d/intake/config.py#L152) function does not handle any exception or error thrown by `ast.literal_eval()` and instead tries to assign a not defined variable to the config dictionary, which causes the error.

It's possible to work around this by not working with local environment variables but I think this should be something one can do. Or at least could do with version 1?

I'm open to work on this problem but would love some insight and guidance on where to start. Or if I'm using local environment variables the wrong way?

Best Johannes

Discussion:
@martindurant: You are right, anything that doesn't parse should probably be returned as a string (perhaps with a config option to raise).

You are right, that this is not an issue for intake2, where the assumption is, that you would use fsspec's file-caching and pass relevant arguments to that. However, if you are using the global variable, then perhaps you are wanting to cache everything, which is not a foreseen use-case. 

It is generally possible to cache datasets (as opposed to files too), and complex pipeline logic can be built up on when to do this. [This test](https://github.com/intake/intake/blob/master/intake/readers/tests/test_workflows.py#L134) would be a good starting point, but we haven't built out all the convenient pipeline elements to make this convenient.@dprovder: Working on this type of functionality for DuckDB. please excuse the mess, I just wrote & tried this out last night.... 

The idea was to use the .token from whatever reader returns the pandas df as the table name in duckdb so that we could detect when the entry has already been read, but I couldn't quite figure our how to get that right. I'll incorporate some of the patterns from the example next so as to clean up some of this redundant conditional logic.

```
class PandasToDuck(BaseConverter):
    instances = {"pandas:DataFrame": "intake.readers.datatypes:SQLQuery"}
    func = "duckdb:DuckDBPyConnection.from_df"

    def run(self, x, url=None, table_name='', save=True, *args, **kwargs):
        """
        Convert a Pandas DataFrame to a DuckDB table, optionally saving it to a database.

        Parameters:
        ----------
        x : pandas.DataFrame
            The Pandas DataFrame to convert.
        url : str, optional
            Path to the DuckDB database (default: in-memory).
        table_name : str, optional
            Name of the table in DuckDB (default: a generated token name).
        save : bool, optional
            Whether to save the table to the DuckDB database (default: False).

        Returns:
        -------
        SQLQuery
        """
        import duckdb
        #from intake.readers import DuckSQL
        from intake.readers.datatypes import SQLQuery

        # Generate a default table name if not provided
        table_name = self.token if not table_name else table_name

        # Persistent DuckDB handling
        conn = duckdb.connect(url)
        print(f"Working with database at: {url}")
        tables = conn.sql('select * from information_schema.tables').table_name.fetchall()
        table_exists = any([(table_name in t[0]) for t in tables])
        if table_exists:
            query = f"SELECT * FROM {table_name}"
            result = SQLQuery({'database': url}, query)
            del result.url
            del result.options
            return result
        else:
            if save:
                        # In-memory DuckDB handling
                if url is None:
                    print("No URL provided; using in-memory DuckDB")
                    relation = duckdb.from_df(x)
                    
                    print(f"Saving table '{table_name}' to in-memory DuckDB")
                    q = f"CREATE OR REPLACE TABLE '{table_name}' AS SELECT * FROM relation"
                    
                    result = SQLQuery({'database': ':in-memory:'}, query)
                    del result.url
                    del result.options
                    return result
                        
                else:
                    # Save the DataFrame as a table in the DuckDB database
                    print(f"Saving DataFrame to table '{table_name}' in DuckDB")
                    conn.register(table_name, x)
                    conn.execute(f"CREATE OR REPLACE TABLE '{table_name}' AS SELECT * FROM '{table_name}'")
                    conn.unregister(table_name)
            
            query = f"SELECT * FROM {table_name}"
            result = SQLQuery({'database': url}, query)
            del result.url
            del result.options
            return result
```@jroettenbacher: To your first point:
Should it raise a Warning or an Error?

To your second point:
So the use of a environment variable starting with INTAKE_ is discouraged and one should instead give the local cache path explicitly in the reader function by providing the simplecache dictionary?
Example assuming use of intake_xarray:
```python
ds = cat[dataset](storage_options=dict(
    simplecache=dict(
        cache_storage='./local/path'
        )
    )
).to_dask()
```

That's great to know! Maybe I'll take a shot at it.@martindurant: Yes, you could do it like that, but fsspec also has a config system allowing you to specify the default value of cache_storage to the simplecache backend. 
```
$ FSSPEC_SIMPLECACHE_CACHE_STORAGE="/Users/mdurant" ipython
In [1]: import fsspec
In [2]: fs, _ = fsspec.url_to_fs("simplecache::file")
In [3]: fs.storage
Out[3]: ['/Users/mdurant']
```@martindurant: > Should it raise a Warning or an Error?

I would say, that the default is to keep a string anything that doesn't parse, and there can be options to warn or to raise. @jroettenbacher: Should we add something about caching in the documentation?
@jroettenbacher: > > Should it raise a Warning or an Error?
> 
> I would say, that the default is to keep a string anything that doesn't parse, and there can be options to warn or to raise.

Okay, I will try and adjust the function and open a pull request for further discussions on the implementation.@martindurant: > Should we add something about caching in the documentation?

Always! :)

Solution patch:
diff --git a/intake/config.py b/intake/config.py
index 0bdbbfba6..c730ee38f 100644
--- a/intake/config.py
+++ b/intake/config.py
@@ -13,6 +13,7 @@
 import logging
 import os
 import posixpath
+import warnings
 from os.path import expanduser
 
 import yaml
@@ -35,6 +36,7 @@
     "extra_imports": [],
     "import_block_list": [],
     "reader_avoid": [],
+    "environment_conf_parse": "ignore",  # "error", "warn", "ignore"
 }
 
 
@@ -46,6 +48,14 @@ class Config(dict):
     """Intake's dict-like config system
 
     Instance ``intake.conf`` is globally used throughout the package
+
+    Attributes:
+        environment_conf_parse : str
+                                 "ignore" (default), "warn" or raise an "error"
+                                 when parsing local environment variables as strings.
+        #TODO: Add other config values
+
+
     """
 
     def __init__(self, filename=None, **kwargs):
@@ -157,8 +167,19 @@ def load_env(self):
                 k2 = k[7:].lower()
                 try:
                     val = ast.literal_eval(v)
-                except ValueError:
-                    pass
+                except (ValueError, SyntaxError) as e:
+                    if self["environment_conf_parse"] == "error":
+                        logger.info(
+                            f"Environment variable '{v}' cannot be parsed by ast.literal_eval()"
+                        )
+                        raise e
+                    elif self["environment_conf_parse"] == "warn":
+                        warnings.warn(
+                            f"Failed to parse environment variable '{k}' (value: '{v}') using ast.literal_eval()."
+                            f"\nParsing as string."
+                            f"\nError was: '{type(e).__name__}: {e}'"
+                        )
+                    val = str(v)  # make sure it is a string
                 self[k2] = val
 
 


Test patch:
diff --git a/intake/tests/test_config.py b/intake/tests/test_config.py
index 127c420a2..0bca17188 100644
--- a/intake/tests/test_config.py
+++ b/intake/tests/test_config.py
@@ -38,3 +38,34 @@ def test_pathdirs():
         "memory://path1",
         "memory://path2",
     ]
+
+
+@pytest.mark.parametrize(
+    "conf",
+    [
+        {"environment_conf_parse": "error"},
+        {"environment_conf_parse": "warn"},
+        {"environment_conf_parse": "ignore"},
+    ],
+)
+def test_load_env(conf):
+    # test the parsing of environment variables as strings
+    os.environ["INTAKE_CACHE"] = "./tmp"  # this causes a SyntaxError
+    os.environ["INTAKE_CACHE2"] = "tmp"  # this causes a ValueError
+    with temp_conf(conf) as fn:
+        if conf["environment_conf_parse"] == "error":
+            # When raise_on_error is True, ensure the exception is raised
+            with pytest.raises(ValueError, SyntaxError):
+                Config(fn)
+        elif conf["environment_conf_parse"] == "warn":
+            # When raise_on_error is False, ensure the variable is parsed as a string
+            with pytest.warns(UserWarning, match="environment variable"):
+                config = Config(fn)
+            assert config["cache"] == "./tmp"
+            assert config["cache2"] == "tmp"
+        else:
+            with pytest.warns(None) as warnings:
+                config = Config(fn)
+            assert not warnings
+            assert config["cache"] == "./tmp"
+            assert config["cache2"] == "tmp"