Skip to content

Runrms add version listing#480

Merged
jcrivenaes merged 2 commits intoequinor:masterfrom
jcrivenaes:runrms-add-version-listing
May 27, 2022
Merged

Runrms add version listing#480
jcrivenaes merged 2 commits intoequinor:masterfrom
jcrivenaes:runrms-add-version-listing

Conversation

@jcrivenaes
Copy link
Copy Markdown
Contributor

Some background:

The runrms script will soon be an replacement for rms for the common user. I.e. /global/distbin/rms will link to /prog/res/roxapi/bin/runrms wrapper which in turn links up the Komodo env and then launch runrms from subscript.

Behind the scene, runrms will still use /prog/roxar/rms/rms and the PR here add a few options that the original script has:

  • Listing the available RMS versions
  • Add seed as option

@jcrivenaes jcrivenaes requested a review from berland May 23, 2022 07:09
@ertomatic
Copy link
Copy Markdown

Can one of the admins verify this patch?

@jcrivenaes jcrivenaes requested a review from vkip May 23, 2022 07:09
Comment thread src/subscript/runrms/runrms.py Outdated
Comment thread src/subscript/runrms/runrms.py Outdated
Comment thread src/subscript/runrms/runrms.py Outdated
Comment thread src/subscript/runrms/runrms.py Outdated
@jcrivenaes jcrivenaes requested a review from berland May 24, 2022 10:44
Comment thread src/subscript/runrms/runrms.py
Comment thread src/subscript/runrms/runrms.py Outdated
Comment thread src/subscript/runrms/runrms.py
Comment thread src/subscript/runrms/runrms.py Outdated
self.lockf = line.replace("\n", "")
self.locked = True
break
lockfiletxt = (pathlib.Path(self.project) / "project_lock_file").read_text()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

pylint will complain here about missing encoding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Did not notice that but have added encoding

parsed_args.project = parsed_args.project_alt

if parsed_args.project and parsed_args.project_alt:
xwarn("Conflict use two types of project input. First argument will win!")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this not be a hard error?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this is buggy, you will trigger this warning when doing only runrms --project foo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Corrected to elif on line 306

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can keep as a soft warning

Comment thread src/subscript/runrms/runrms.py Outdated
"--project",
"-project",
dest="project_alt",
help="RMS project name (alternative version)",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is "alternative version" sufficient for explaining users of runrms --help that there are two ways of supplying the project name, or should it be explained more?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea was full backwards user experience with current rms -project foo. I will explain more

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Detailed explanation added

@jcrivenaes jcrivenaes requested a review from berland May 26, 2022 20:58
Copy link
Copy Markdown
Collaborator

@berland berland left a comment

Choose a reason for hiding this comment

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

👍🏻

@jcrivenaes jcrivenaes merged commit f735f74 into equinor:master May 27, 2022
@jcrivenaes jcrivenaes deleted the runrms-add-version-listing branch May 27, 2022 07:18
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.

3 participants