config fails to initialize

[edublancas]

I saw this in this example:

/home/docs/checkouts/readthedocs.org/user_builds/sklearn-evaluation/conda/latest/lib/python3.8/site-packages/ploomber_core/config.py:31: UserWarning: Error loading '/home/docs/.ploomber/stats/config.yaml': 'NoneType' object has no attribute 'get'

reverting to default values
  warnings.warn(f'Error loading {str(path)!r}: {e}\n\n'
/home/docs/checkouts/readthedocs.org/user_builds/sklearn-evaluation/conda/latest/lib/python3.8/site-packages/ploomber_core/config.py:31: UserWarning: Error loading '/home/docs/.ploomber/stats/uid.yaml': 'NoneType' object has no attribute 'get'

reverting to default values
  warnings.warn(f'Error loading {str(path)!r}: {e}\n\n'
/home/docs/checkouts/readthedocs.org/user_builds/sklearn-evaluation/conda/latest/lib/python3.8/site-packages/ploomber_core/config.py:31: UserWarning: Error loading '/home/docs/.ploomber/stats/config.yaml': 'NoneType' object has no attribute 'get'

reverting to default values

looks like the config fails to initialize and the file is never created

Maybe this is the right thing to do and it's just that some logic is wrong. I updated this package so it doesn't call posthog when it detects it's running on readthedocs (which we use to generate documentation) - perhaps that's causing the warning?

[edublancas]

hint to reproduce: let's simulate that something is running on READTHEDOCS by setting the env variable , ensure the ~/.ploomber directory does not exist and then see if running some ploomber example triggers that warning

[yafimvo]

I've cloned this repo and executed the code according to the example and the steps to reproduce (with READTHEDOCS as env var).

I got the same warning (6 times)
reverting to default values warnings.warn(f'Error loading {str(path)!r}: {e}\n\n' C:\Users\User\Anaconda3\envs\ploomber-core\lib\site-packages\ploomber_core\config.py:31: UserWarning: Error loading 'C:\\Users\\User\\.ploomber\\stats\\config.yaml': 'NoneType' object has no attribute 'get'

Working directory

I ran the same example using the serial executor and didn't get this warning.

The number of newly created files is the same in both, parallel and serial.

It seems the warning comes from here when content is 'None' (and it happens only when using parallel)

[edublancas]

does the error still happen if you don't set the READTHEDOCS?

any conclusions why it happens with parallel? You might need to add some logging to debug this.

[edublancas]

so I noticed something, the error is saying the object is None, this happens when the YAML is an empty file. So looks like the file exists, but it's empty. maybe a race condition since multiple processes are trying to save the same file?

[yafimvo]

The problem persists even without setting READTHEDOCS.
I debugged it in Parallel and Serial with some logs and it seems like it is a race condition situation (i.e the number of these warning is changing from time to time).

I'm not sure where to begin to fix this. (I thought it will be in Parallel class here but it never gets there).

[edublancas]

I think this might be a sklearn-evaluation-specific problem and have an idea of how to debug. Let me try something quick in the next few days and I'll post an update here

[edublancas]

update: this looks like a general problem. I thought it was only happening on sklearn-evaluation but it also happens when using ploomber. so please @yafimvo help us with this!

[yafimvo]

@edublancas
I added a lock to TaskBuildWrapper

    def __call__(self, lock, *args, **kwargs):
        with lock:
            try:
                output = self.task._build(**kwargs)
                return output
            except Exception as e:
                return Message(task=self.task,
                               message=_format.exception(e), obj=e)

So we can use it like this

future = pool.submit(TaskBuildWrapper(task), lock, **task_kwargs)

It seems to fix these warnings.

Some notes:

1. I'm not sure how to auto-test this
2. This change doesn't break existing tests
3. If this is an acceptable solution I'll create a PR to Ploomber repo

[edublancas]

what is this lock doing? wouldn't it interfere with the parallel executor logic? i.e.g, if a task acquires a lock, other tasks won't run simultaneously?

did you find out why this is happening? It seems to me that the error is in this package, and a fix should be applied here

[yafimvo]

what is this lock doing? wouldn't it interfere with the parallel executor logic? i.e.g, if a task acquires a lock, other tasks won't run simultaneously?

This lock should protect config.yaml and uid.yaml from being changed by other tasks. Even though it passed the existing tests, this implementation locks the entire task, so yes, I think this may affect the parallel executor (the lock will be released once the task has been completed).

did you find out why this is happening? It seems to me that the error is in this package, and a fix should be applied here

I think it happens when 2 tasks run simultaneously, one creates the configuration file (config.yaml/uid.yaml), while the other task is trying to read from it.

I think it completes the tasks successfully since we have this exception handler to re-load content from the dictionary

            try:
                content = self._load_from_file()
                loaded = True
            except Exception as e:
                warnings.warn(f'Error loading {str(path)!r}: {e}\n\n'
                              'reverting to default values')
                loaded = False
                content = self._get_data()

Can we just use the content = self._get_data() instead of reading from the file?

[edublancas]

I think this may affect the parallel executor (the lock will be released once the task has been completed).

this is a big drawback for users, so we'll have to find another solution

Can we just use the content = self._get_data() instead of reading from the file?

ah. yes! I started overcomplicating, thinking we'd need a more elaborate solution, but you're right! This problem happens because the first process doesn't see the file, so it proceeds to create it, while the second sees the file but not its contents (since the writing operation is still in progress). However, under that scenario, it's safe to load self._get_data, since it'll load the default values (which are the same values that we're writing).

I wanted to convince myself and tested the solution. I think it works but please review #12

[yafimvo]

Question: I noticed that configuration files might differ from task to task. When loading data directly from a file, are we sure it's the right configuration for this task?

[edublancas]

Question: I noticed that configuration files might differ from task to task. When loading data directly from a file, are we sure it's the right configuration for this task?

Yes, Config is an abstract class with an abstract path method. Concrete classes define this method which returns the name of the file they're using:

core/src/ploomber_core/config.py

Line 116 in e6e5cc3

def path(cls):