pyscalfile is added to prepend_root_path_to_relative_files() for inte…

[bkhegstad]

See issue 473. I did not get any response - so here is a suggestion for updates.

[ertomatic]

Can one of the admins verify this patch?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.57%. Comparing base (48a1437) to head (372942d).

Files with missing lines	Patch %	Lines
src/subscript/interp_relperm/interp_relperm.py	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
- Coverage   87.74%   87.57%   -0.18%     
==========================================
  Files          49       49              
  Lines        6995     7024      +29     
==========================================
+ Hits         6138     6151      +13     
- Misses        857      873      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[asnyv]

Hi @bkhegstad, I think this looks good. The only issue I see is that there is a slight risk that someone might have based their workflow on the inconsistency, so that results may be affected? But the probability is likely so low that we could consider to take that risk.

The scenarios I think of are as follows (all of them assuming that the root_path option is set and a relative path is defined for pyscalfile):

1. The pyscalfile is copied in the ERT workflow to the realizations, but the same relative path doesn't fit any file from your defined root_path. (Will break the workflow, but at least not getting wrong results)
2. The pyscalfile is copied in the ERT workflow to the realizations, and the same relative path fits a file from your defined root_path, but it is not the same file (e.g. you have gotten new pvt, and called the file something else). Will not break the workflow, but affect results (not good!)

So it could technically be a breaking change for some (without them noticing), meaning that we have to think twice before we merge.
Should also ideally add a test for this.

@berland any thoughts?

[berland]

shooting almost from the hip: probably ok!

[bkhegstad]

If I understand the script correctly:

1. pyscalfile is an alternative to using base/low/high. One should use either pyscalfile or base/low/high
2. If someone is using base/low/high the proposed update will not change the results
3. If someone is using pyscalfile - most likely they do not use root-path - since it does not have any effect

So as far as I can see - the only way it can go wrong is someone using pyscalfile AND is using --root-path which has no effect today and is wrong. Then suddenly --root-path will work and will have an effect. This is not likely - and will most likely give an error. @asnyv, @berland

[asnyv]

ah, yes that seems likely to be the case @bkhegstad, think you have studied the code deeper that I have 😉 But in that case we are probably quite safe 😊
From what I can see with a quick look, I cannot see that you get any warning/error if defining both pyscalfile and base/low/high though? In that case I think we should introduce that to avoid that users misconfigure it? Should probably fail hard if both are configured to avoid misunderstanding? But could be that I just didn't spot it.

[berland]

These things usually becomes clear if one writes tests to solidify the current behaviour, including the wrong usage patterns. Experience tells that more related bugs will pop up if these tests are made.

[bkhegstad]

I can add a simple test making sure that both pyscalfile and base/low/high cannot be defined in the same model-file @asnyv

[lindjoha]

Can we merge this PR into master? It is problematic to use absolute file paths in these files since all user copies are pointing to the master project. This is obviously not the behaviour we want. @berland @bkhegstad @asnyv

[asnyv]

@lindjoha as long as we fix what crashes the CI (looks like just some import sorting), I'm ok with merging :)

[bkhegstad]

@asnyv: All checks in CI have passed now. Please merge

@lindjoha

[lindjoha]

@asnyv: All checks in CI have passed now. Please merge

@lindjoha

Can you approve @asnyv ?

[asnyv]

LGTM