Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix res import errors #2502

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Fix res import errors #2502

merged 2 commits into from
Dec 7, 2021

Conversation

eivindjahren
Copy link
Contributor

@eivindjahren eivindjahren commented Dec 6, 2021

Issue

#2503

Approach

  • Adds __all__ to __init__
  • Import implicit dependencies in the module it is used
  • Use typing to make the use of the import explicit
  • Remove unused dependencies

Several files would rely on importing from the __init__.py of its module. This is quite fragile as the order of imports in the __init__.py must facilitate a partial instantiation in order to avoid circular imports. As more imports are added to resolve the above issue, it becomes very difficult to continue with that scheme. Therefore imports such as from . import EnkFMain is changed to from .enkf_main import EnkFMain for any file inside res/enkf while files outside res/enkf can use from res.enkf import EnkFMain.

@eivindjahren eivindjahren force-pushed the fix_res_imports branch 22 times, most recently from 8e53b29 to 7e39367 Compare December 7, 2021 07:55
@eivindjahren eivindjahren marked this pull request as ready for review December 7, 2021 07:55
@eivindjahren eivindjahren force-pushed the fix_res_imports branch 4 times, most recently from 4180273 to be04ee3 Compare December 7, 2021 10:35
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #2502 (bea3bb7) into main (e6d8a65) will increase coverage by 0.02%.
The diff coverage is 96.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2502      +/-   ##
==========================================
+ Coverage   63.74%   63.76%   +0.02%     
==========================================
  Files         658      658              
  Lines       54900    54954      +54     
  Branches     4393     4393              
==========================================
+ Hits        34994    35041      +47     
- Misses      18582    18588       +6     
- Partials     1324     1325       +1     
Impacted Files Coverage Δ
res/enkf/export/design_matrix_reader.py 100.00% <ø> (ø)
res/enkf/local_obsdata_node.py 86.48% <ø> (ø)
res/enkf/node_id.py 72.72% <ø> (ø)
res/enkf/plot/__init__.py 0.00% <0.00%> (ø)
res/enkf/plot/ensemble_block_data_fetcher.py 0.00% <0.00%> (ø)
res/enkf/plot/ensemble_data_fetcher.py 0.00% <0.00%> (ø)
res/enkf/plot/refcase_data_fetcher.py 0.00% <0.00%> (ø)
res/enkf/enkf_state.py 73.68% <72.72%> (-0.39%) ⬇️
res/enkf/data/gen_kw.py 90.36% <80.00%> (-1.11%) ⬇️
res/analysis/__init__.py 100.00% <100.00%> (ø)
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d8a65...bea3bb7. Read the comment docs.

@@ -31,7 +34,7 @@ class History(BaseCClass):
)
_free = ResPrototype("void history_free( history )")

def __init__(self, refcase=None, use_history=False, sched_file=None):
def __init__(self, refcase=Optional[EclSum], use_history=False, sched_file=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __init__(self, refcase=Optional[EclSum], use_history=False, sched_file=None):
def __init__(self, refcase: Optional[EclSum] = None, use_history=False, sched_file=None):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, nice catch. Fixed

@@ -54,7 +56,7 @@ def __init__(self, ensemble_config_node, file_system, input_mask=None):

self.__load(file_system, input_mask)

def __load(self, file_system, input_mask=None):
def __load(self, file_system: EnkfFs, input_mask: BoolVector = None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def __load(self, file_system: EnkfFs, input_mask: BoolVector = None):
def __load(self, file_system: EnkfFs, input_mask: Optional[BoolVector] = None):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now

self, fs, param_list=None, init_mode=EnkfInitModeEnum.INIT_CONDITIONAL
self,
fs: EnkfFs,
param_list: StringList = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
param_list: StringList = None,
param_list: Optional[StringList] = None,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now

def __init__(
self,
user_config_file=None,
config_content: ConfigContent = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
config_content: ConfigContent = None,
config_content: Optional[ConfigContent] = None,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now

* Adds __all__ to __init__
* Use typing to enforce use of silent dependencies
* Remove base dependencies that are unused

Several files would rely on importing from the __init__.py of its
module. This is quite fragile as the order of imports in the __init__.py
must facilitate a partial instantiation in order to avoid circular
imports. As more imports are added to resolve the above issue, its no
longer possible to continue with that scheme. Therefore imports such as
from . import EnkFMain is changed to from .enkf_main import EnkFMain for
any file in res/enkf while files outside res/enkf can use from res.enkf
import EnkFMain.
@eivindjahren eivindjahren merged commit 237d3e9 into main Dec 7, 2021
@eivindjahren eivindjahren deleted the fix_res_imports branch December 7, 2021 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants