Add migration from BlockFs storage #4937

pinkwah · 2023-02-22T12:44:50Z

No description provided.

codecov-commenter · 2023-03-13T10:53:48Z

Codecov Report

Merging #4937 (1aff5b5) into main (eddeded) will decrease coverage by 0.79%.
The diff coverage is 22.82%.

@@            Coverage Diff             @@
##             main    #4937      +/-   ##
==========================================
- Coverage   77.88%   77.09%   -0.79%     
==========================================
  Files         391      393       +2     
  Lines       25545    25926     +381     
  Branches     1675     1760      +85     
==========================================
+ Hits        19895    19987      +92     
- Misses       5064     5353     +289     
  Partials      586      586

Impacted Files	Coverage Δ
src/clib/lib/block_fs_native/module.cpp	`0.00% <0.00%> (ø)`
src/ert/_c_wrappers/enkf/ensemble_config.py	`96.49% <ø> (ø)`
src/ert/storage/migration/block_fs.py	`22.29% <22.29%> (ø)`
src/ert/storage/local_storage.py	`89.94% <73.68%> (-8.31%)`	⬇️
src/ert/_c_wrappers/enkf/config/surface_config.py	`100.00% <100.00%> (ø)`
src/ert/cli/main.py	`91.30% <100.00%> (+0.19%)`	⬆️
src/ert/gui/main.py	`92.82% <100.00%> (+2.32%)`	⬆️
src/ert/storage/__init__.py	`91.30% <100.00%> (ø)`
src/ert/storage/local_ensemble.py	`96.27% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

oyvindeide

Good job! A minor question and nitpicky comment

oyvindeide · 2023-05-24T10:37:23Z

src/clib/lib/block_fs_native/module.cpp

+    size_t len;
+    size_t count;
+
+    bool is_compressed() const {


Does not seem to be used?

oyvindeide · 2023-05-24T10:46:34Z

src/ert/storage/local_storage.py

+        self.path = Path(path)
+        if not ignore_migration_check and local_storage_needs_migration(self.path):
+            from ert.storage.migration.block_fs import migrate  # pylint: disable=C0415
+
+            migrate(self.path)


Not used at the moment, perhaps turn this migration into an issue instead? I would suggest moving this to the main entry point instead, so we have more control of the migration.

It would be used in production (if storage needs migration, migrate)

Should be optional though? So you get a prompt, saying would you like to delete or migrate?

tests/unit_tests/storage/migration/test_block_fs_simple_case.py

oyvindeide · 2023-05-24T11:01:57Z

tests/unit_tests/storage/migration/test_block_fs_simple_case.py

+    ensemble = experiment.create_ensemble(name="default", ensemble_size=5)
+    bf._migrate_field(ensemble, parameter, ens_config)
+
+    for key, data in data["/REAL_0/FIELD"].groups.items():


Would it be possible to parameterize this instead? Cant immediately see how, but think it might be possible.

I can metaprogram myself to a solution, ie:

@pytest.mark.parameterize("type", ["field", "surface"]) def test_stuff(type): getattr(bf, f"_migrate_{type}")(ensemble, parameter, ens_config) for key, data in data[f"/REAL_0/{type.upper()}"].groups.items(): ...

Do we want to use getattr in this way?

No, fine to leave it as is

src/ert/storage/local_storage.py

dafeda · 2023-05-26T07:09:50Z

What are the benefits of using C++ for this?

dafeda · 2023-05-30T08:10:27Z

src/clib/lib/block_fs_native/module.cpp

+    }
+}
+
+auto parse_name(const std::string &name, Kind kind)


What does name refer to here?
Also, seems like parse_name is only tested for Kind::SUMMARY.
Is it worthwhile adding tests for other kinds as well?

Yes. I just forgot to do so.

dafeda · 2023-05-30T10:00:54Z

src/clib/lib/block_fs_native/module.cpp

+                m_stream.read(name.data(), name.size());
+                skip<char>(m_stream); // NULL terminator
+
+                auto node_size = read_i32(m_stream, nullptr);


annotate-cpp FTW :)

annotate-cpp complained that node_size was not used.

dafeda · 2023-05-31T06:40:24Z

src/clib/lib/block_fs_native/module.cpp

+    std::ifstream m_stream;
+    /** Stream mutex */
+    mutable std::mutex m_mutex;
+    /** List of SUMMARY blocks */


I think comment is outdated as m_blocks holds other kinds of blocks as well.

dafeda · 2023-05-31T07:35:37Z

src/ert/storage/local_storage.py

+        return False
+
+    try:
+        with open("index.json", encoding="utf-8") as f:


Is it perhaps better to use _Index.parse_file here as well?

It might not be. We are only interested in the version key because the structure of index.json is unknown at this time. _Index.parse_file would imply that the structure of index.json is the same as it is currently, which may not be the case in the future.

dafeda · 2023-05-31T07:49:31Z

src/ert/storage/local_storage.py

+            return False
+        if version < _LOCAL_STORAGE_VERSION:
+            return True
+        raise NotImplementedError("Incompatible ERT Local Storage")


Could we be more explicit here and say that version > _LOCAL_STORAGE_VERSION or something like that?

Your wish is my command

src/ert/storage/local_storage.py

dafeda · 2023-05-31T12:58:14Z

Should these steps be a happy path?

Install 4.5.6 which is what's in stable.
Run ert ensemble_smoother poly.ert --target_case "es_update"
Install this branch
Run ert gui poly.ert

Exception ignored in: <function LocalStorageReader.__del__ at 0x16370a320>
Traceback (most recent call last):
  File "/Users/FCUR/git/ert/src/ert/storage/local_storage.py", line 142, in __del__
    self.close()
  File "/Users/FCUR/git/ert/src/ert/storage/local_storage.py", line 189, in close
    super().close()
  File "/Users/FCUR/git/ert/src/ert/storage/local_storage.py", line 70, in close
    for ensemble in self._ensembles.values():
AttributeError: 'LocalStorageAccessor' object has no attribute '_ensembles'
ERT crashed unexpectedly with "Use 'local_storage_set_ert_config' before retrieving the config".
See logfile(s) for details:
   /Users/FCUR/git/poly_example/logs/ert-log-2023-05-31T1455.txt

dafeda · 2023-06-01T06:10:14Z

I redid the experiment previously defined and now get this:

Traceback (most recent call last):
  File "/Users/FCUR/envs/intel/.venvfmu/bin/ert", line 33, in <module>
    sys.exit(load_entry_point('ert', 'console_scripts', 'ert')())
  File "/Users/FCUR/envs/intel/.venvfmu/bin/ert", line 25, in importlib_load_entry_point
    return next(matches).load()
  File "/usr/local/Homebrew/Cellar/python@3.10/3.10.11/Frameworks/Python.framework/Versions/3.10/lib/python3.10/importlib/metadata/__init__.py", line 171, in load
    module = import_module(match.group('module'))
  File "/usr/local/Homebrew/Cellar/python@3.10/3.10.11/Frameworks/Python.framework/Versions/3.10/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/Users/FCUR/git/ert/src/ert/__main__.py", line 26, in <module>
    from ert.cli.main import ErtCliError, ErtTimeoutError, run_cli
  File "/Users/FCUR/git/ert/src/ert/cli/main.py", line 28, in <module>
    from ert.storage import StorageAccessor, local_storage_set_ert_config, open_storage
ImportError: cannot import name 'local_storage_set_ert_config' from 'ert.storage' (/Users/FCUR/git/ert/src/ert/storage/__init__.py)

dafeda

I tested also with running es-mda on snake_oil_field with ert version 4.5.6 and then with this branch.
Seems to work.

Stellar work and good to go I think 👍

pinkwah self-assigned this Feb 22, 2023

pinkwah force-pushed the migration branch 4 times, most recently from 69157fd to 15c6162 Compare February 22, 2023 14:51

oyvindeide force-pushed the enkf_fs_refactor branch from 74b699d to c0eb4d5 Compare February 24, 2023 16:21

pinkwah force-pushed the migration branch 2 times, most recently from aa277a5 to 8ac4494 Compare February 27, 2023 14:40

pinkwah force-pushed the enkf_fs_refactor branch from 7095ceb to 491ec4d Compare March 3, 2023 12:42

oyvindeide force-pushed the enkf_fs_refactor branch from f0aa918 to fed5b18 Compare March 6, 2023 11:46

pinkwah force-pushed the migration branch 2 times, most recently from 59c713f to 716dfcb Compare March 13, 2023 10:36

pinkwah force-pushed the migration branch from 716dfcb to c7c4fea Compare March 22, 2023 11:14

pinkwah changed the base branch from enkf_fs_refactor to main March 22, 2023 11:14

pinkwah force-pushed the migration branch from fd4d603 to 1ef0346 Compare March 28, 2023 12:38

pinkwah force-pushed the migration branch from 324e3bc to ccf9d81 Compare April 18, 2023 12:31

pinkwah force-pushed the migration branch 13 times, most recently from 9a08e68 to 6a216f9 Compare May 3, 2023 13:19

oyvindeide reviewed May 24, 2023

View reviewed changes

tests/unit_tests/storage/migration/test_block_fs_simple_case.py Outdated Show resolved Hide resolved

oyvindeide reviewed May 24, 2023

View reviewed changes

dafeda reviewed May 26, 2023

View reviewed changes

src/ert/storage/local_storage.py Outdated Show resolved Hide resolved

dafeda reviewed May 30, 2023

View reviewed changes

pinkwah force-pushed the migration branch 3 times, most recently from 842c443 to 964018e Compare May 30, 2023 08:34

dafeda reviewed May 30, 2023

View reviewed changes

pinkwah force-pushed the migration branch 5 times, most recently from 51ae460 to 7a3952b Compare May 30, 2023 12:54

dafeda reviewed May 31, 2023

View reviewed changes

src/ert/storage/local_storage.py Show resolved Hide resolved

pinkwah force-pushed the migration branch from 7a3952b to f039657 Compare May 31, 2023 08:19

pinkwah force-pushed the migration branch from f039657 to 3614253 Compare May 31, 2023 13:17

pinkwah force-pushed the migration branch 2 times, most recently from dcd3db2 to bfc4590 Compare June 1, 2023 08:05

Add migration from BlockFs storage

1aff5b5

pinkwah force-pushed the migration branch from bfc4590 to 1aff5b5 Compare June 1, 2023 08:07

dafeda approved these changes Jun 1, 2023

View reviewed changes

pinkwah merged commit 9e65975 into equinor:main Jun 1, 2023
37 checks passed

pinkwah deleted the migration branch June 1, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add migration from BlockFs storage #4937

Add migration from BlockFs storage #4937

pinkwah commented Feb 22, 2023

codecov-commenter commented Mar 13, 2023 •

edited

oyvindeide left a comment

oyvindeide May 24, 2023

oyvindeide May 24, 2023

pinkwah May 30, 2023

oyvindeide May 31, 2023

oyvindeide May 24, 2023

pinkwah May 26, 2023

oyvindeide May 31, 2023

dafeda commented May 26, 2023

dafeda May 30, 2023

pinkwah May 30, 2023

dafeda May 30, 2023

dafeda May 30, 2023

dafeda May 31, 2023

dafeda May 31, 2023

pinkwah May 31, 2023

dafeda May 31, 2023

pinkwah May 31, 2023

dafeda commented May 31, 2023

dafeda commented Jun 1, 2023

dafeda left a comment

Add migration from BlockFs storage #4937

Add migration from BlockFs storage #4937

Conversation

pinkwah commented Feb 22, 2023

codecov-commenter commented Mar 13, 2023 • edited

Codecov Report

oyvindeide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dafeda commented May 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dafeda commented May 31, 2023

dafeda commented Jun 1, 2023

dafeda left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 13, 2023 •

edited