Repository: AgnostiqHQ/covalent
Pull request #1719 (2023-07-04 15:21:00)

Fix for config file locking

<!--
⚠️ Your pull request title should be short detailed and understandable for all.
⚠️ Add a note to /CHANGELOG.md summarizing the changes.
⚠️ If your pull request fixes an open issue, please link to the issue.
⚠️ Ensure that your branch is up-to-date with the base branch.
-->

- [ ] I have added the tests to cover my changes.
- [x] I have updated the documentation and CHANGELOG accordingly.
- [x] I have read the CONTRIBUTING document.


github-actions[bot]: Hello. You may have forgotten to update the changelog!
Please edit [`CHANGELOG.md`](/AgnostiqHQ/covalent/blob/master/CHANGELOG.md) with a one-to-two sentence description of the change in the UNRELEASED section. You may include a small working example for new features.
arosen93: Closes #1670.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/AgnostiqHQ/covalent/pull/1719?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) Report
> Merging [#1719](https://app.codecov.io/gh/AgnostiqHQ/covalent/pull/1719?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) (767b53b) into [develop](https://app.codecov.io/gh/AgnostiqHQ/covalent/commit/7e7bf51c5c319833e1dc0bc371a1ed86df12a6e3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) (7e7bf51) will **increase** coverage by `2.01%`.
> The diff coverage is `n/a`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/AgnostiqHQ/covalent/pull/1719/graphs/tree.svg?width=650&height=150&src=pr&token=YGHCB3DE4P&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ)](https://app.codecov.io/gh/AgnostiqHQ/covalent/pull/1719?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ)

```diff
@@             Coverage Diff             @@
##           develop    #1719      +/-   ##
===========================================
+ Coverage    88.10%   90.12%   +2.01%     
===========================================
  Files          161       85      -76     
  Lines         6962     5244    -1718     
  Branches       151        0     -151     
===========================================
- Hits          6134     4726    -1408     
+ Misses         723      518     -205     
+ Partials       105        0     -105     
```

| Flag | Coverage Δ | |
|---|---|---|
| Dispatcher | `87.71% <ø> (ø)` | |
| Functional_Tests | `?` | |
| SDK | `91.49% <ø> (ø)` | |
| UI_Frontend | `?` | |

</details>
arosen93: By the way, not that it needs to be done in this PR, but there's one other section in the codebase that relies on the OS-dependent `fcntl`.

https://github.com/AgnostiqHQ/covalent/blob/691f0ebf57ca6fcfbe48a3b6cd5e2bb79b178400/covalent_ui/api/main.py#L73-L75

Maybe it'll be possible to replace that call too at some point.
kessler-frost: Yep that's on my radar. Thanks @arosen93 !
kessler-frost: @arosen93 Do you mind helping me out a bit with this one, can you check whether this solution works on your side on the slurm cluster for which covalent was halting earlier? Thanks a lot!
arosen93: @kessler-frost: I reran my typical test. A few notes.

1. After pip install-ing, it didn't install filelock. So, I had to do that manually.
2. Covalent no longer freezes on import
3. Jobs do not run successfully by default (traceback below), _but this is expected._

```python
Traceback (most recent call last):
  File "/pscratch/sd/r/rosen/test.py", line 1, in <module>
    import covalent
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/__init__.py", line 26, in <module>
    from . import executor, leptons  # nopycln: import
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/executor/__init__.py", line 33, in <module>
    from .._shared_files import logger
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/_shared_files/logger.py", line 34, in <module>
    log_level = get_config("sdk.log_level").upper()
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/_shared_files/config.py", line 244, in get_config
    cm = ConfigManager()
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/_shared_files/config.py", line 56, in __init__
    self.update_config()
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/covalent/_shared_files/config.py", line 113, in update_config
    with filelock.FileLock(f"{self.config_file}.lock", timeout=1):
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/filelock/_api.py", line 255, in __enter__
    self.acquire()
  File "/global/common/software/matgen/rosen/miniconda/envs/test/lib/python3.10/site-packages/filelock/_api.py", line 222, in acquire
    raise Timeout(lock_filename)  # noqa: TRY301
filelock._error.Timeout: The file lock '/global/homes/r/rosen/.config/covalent/covalent.conf.lock' could not be acquired.
```

The reason the lock can't be acquired is because the default `COVALENT_CONFIG_DIR` (being in `~/`) on Perlmutter doesn't supporting locking mechanisms in general. I don't see a real way of getting around that aside from not using a locking mechanism at all. But more importantly, because there's no stalling and a _proper_ error is raised, at least it is telling the user to do something about it. In this case, the user has to just update their `COVALENT_CONFIG_DIR` to a filesystem that supports locking. I've confirmed that everything works as expected once that's done.

So, in summary, I think this solution is better and worth merging because:
- There is no stalling upon error
- A proper error is raised
- It is an OS-agnostic solution

Just make sure `filelock` makes its way into `setup.py`.
kessler-frost: Thanks a lot @arosen93 ! I'll clean this one up and get this merged.
arosen93: Great! Thanks for addressing this :) 
kessler-frost: ```python
 def set_winsize(fd, row, col, xpix=0, ypix=0): 
     winsize = struct.pack("HHHH", row, col, xpix, ypix) 
     fcntl.ioctl(fd, termios.TIOCSWINSZ, winsize) 
```
The above `fcntl` in `covalent/covalent_ui/api/main.py` can be changed to something else later since it's UI based and requires a different type of testing.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e01a8a71..06610d0a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Using `filelock` package now for platform independent file locking of config file. This should fix the failing tests as well as improve compatibility with Windows.
 - When stopping the server, we send the proper `SIGINT` signal to uvicorn instead of `SIGKILL` which allows the second part of the FastAPI `lifespan` to execute properly.
 - Fixed the outstanding incompatibities between front-end data layer and a postgres database
 - Reverted file-lock changes
diff --git a/covalent/_shared_files/config.py b/covalent/_shared_files/config.py
index 63690305..3bc9c16b 100644
--- a/covalent/_shared_files/config.py
+++ b/covalent/_shared_files/config.py
@@ -18,7 +18,7 @@
 #
 # Relief from the License may be granted by purchasing a commercial license.
 
-import fcntl
+
 import os
 import shutil
 from dataclasses import asdict
@@ -27,6 +27,7 @@ from operator import getitem
 from pathlib import Path
 from typing import Any, Dict, List, Optional, Union
 
+import filelock
 import toml
 
 """Configuration manager."""
@@ -109,16 +110,16 @@ class ConfigManager:
                         else:
                             old_dict.setdefault(key, value)
 
-        with open(self.config_file, "r+") as f:
-            fcntl.lockf(f, fcntl.LOCK_EX)
-            file_config = toml.load(f)
+        with filelock.FileLock(f"{self.config_file}.lock", timeout=1):
+            with open(self.config_file, "r+") as f:
+                file_config = toml.load(f)
 
-            update_nested_dict(self.config_data, file_config)
-            if new_entries:
-                update_nested_dict(self.config_data, new_entries, override_existing)
+                update_nested_dict(self.config_data, file_config)
+                if new_entries:
+                    update_nested_dict(self.config_data, new_entries, override_existing)
 
-            # Writing it back to the file
-            self.write_config()
+                # Writing it back to the file
+                self.write_config()
 
     def read_config(self) -> None:
         """
@@ -143,8 +144,8 @@ class ConfigManager:
         Returns:
             None
         """
+
         with open(self.config_file, "w") as f:
-            fcntl.lockf(f, fcntl.LOCK_EX)
             toml.dump(self.config_data, f)
 
     def purge_config(self) -> None:
diff --git a/requirements-client.txt b/requirements-client.txt
index 65c6547f..2ad35464 100644
--- a/requirements-client.txt
+++ b/requirements-client.txt
@@ -2,6 +2,7 @@ aiofiles>=0.8.0
 aiohttp>=3.8.1
 cloudpickle>=2.0.0
 dask[distributed]>=2022.6.0
+filelock>=3.12.2
 furl>=2.1.3
 networkx>=2.8.6
 requests>=2.24.0
diff --git a/requirements.txt b/requirements.txt
index 8c233258..c7338385 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -5,6 +5,7 @@ click>=8.1.3
 cloudpickle>=2.0.0
 dask[distributed]>=2022.6.0
 fastapi>=0.93.0
+filelock>=3.12.2
 furl>=2.1.3
 natsort>=8.4.0
 networkx>=2.8.6


Test patch:
diff --git a/tests/covalent_tests/shared_files/config_test.py b/tests/covalent_tests/shared_files/config_test.py
index cd772109..e29cef0b 100644
--- a/tests/covalent_tests/shared_files/config_test.py
+++ b/tests/covalent_tests/shared_files/config_test.py
@@ -219,12 +219,34 @@ def test_write_config(mocker):
     cm = ConfigManager()
     toml_dump_mock = mocker.patch("covalent._shared_files.config.toml.dump")
     open_mock = mocker.patch("covalent._shared_files.config.open")
-    lock_mock = mocker.patch("fcntl.lockf")
     mock_file = open_mock.return_value.__enter__.return_value
     cm.write_config()
     toml_dump_mock.assert_called_once_with(cm.config_data, mock_file)
     open_mock.assert_called_once_with(cm.config_file, "w")
-    lock_mock.assert_called_once()
+
+
+def test_update_config(mocker):
+    """Test the update_config method for config manager."""
+
+    cm = ConfigManager()
+
+    cm.config_file = "mock_config_file"
+    cm.config_data = {"mock_section": {"mock_dir": "initial_value"}}
+    # Cannot mock `update_nested_dict`` since it's defined within the function
+
+    mock_filelock = mocker.patch("covalent._shared_files.config.filelock.FileLock")
+    mock_open = mocker.patch("covalent._shared_files.config.open")
+    mock_toml_load = mocker.patch("covalent._shared_files.config.toml.load")
+
+    cm.write_config = mocker.Mock()
+
+    cm.update_config()
+
+    mock_filelock.assert_called_once_with("mock_config_file.lock", timeout=1)
+    mock_open.assert_called_once_with("mock_config_file", "r+")
+    mock_toml_load.assert_called_once_with(mock_open.return_value.__enter__.return_value)
+
+    cm.write_config.assert_called_once()
 
 
 def test_config_manager_set(mocker):
diff --git a/tests/functional_tests/docs_how_to_test.py b/tests/functional_tests/docs_how_to_test.py
index 7ce8f217..57215650 100644
--- a/tests/functional_tests/docs_how_to_test.py
+++ b/tests/functional_tests/docs_how_to_test.py
@@ -46,6 +46,8 @@ ignore_files = [
     "creating_custom_executors.ipynb",
     "file_transfers_to_from_azure_blob.ipynb",
     "file_transfers_to_from_gcp_storage.ipynb",
+    "file_transfers_for_workflows_to_remote.ipynb",
+    "file_transfers_to_remote.ipynb",
 ]
 
 
diff --git a/tests/requirements.txt b/tests/requirements.txt
index fe4fa8d1..29415a9c 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,18 +1,17 @@
-boto3==1.26.110
-detect-secrets==1.3.0
-flake8==5.0.4
-httpx==0.24.1
-isort==5.10.1
-locust==2.11.0
-mock==4.0.3
-nbconvert==6.5.1
-pennylane==0.25.1
-pre-commit==2.20.0
-pytest==7.1.3
-pytest-asyncio==0.21.0
-pytest-cov==3.0.0
-pytest-mock==3.8.2
-pytest-rerunfailures==10.2
-pytest_asyncio==0.21.0
-scikit-image==0.19.1
-scikit-learn==1.2.2
+boto3>=1.26.110
+detect-secrets>=1.3.0
+flake8>=5.0.4
+httpx>=0.24.1
+isort>=5.10.1
+locust>=2.11.0
+mock>=4.0.3
+nbconvert>=6.5.1
+pennylane>=0.25.1
+pre-commit>=2.20.0
+pytest>=7.1.3
+pytest-asyncio>=0.21.0
+pytest-cov>=3.0.0
+pytest-mock>=3.8.2
+pytest-rerunfailures>=10.2
+scikit-image>=0.19.1
+scikit-learn>=1.2.2