-
Notifications
You must be signed in to change notification settings - Fork 15
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
BUG: Use classvar when setting inside-rms flag #414
BUG: Use classvar when setting inside-rms flag #414
Conversation
logger.info("Run from inside RMS (or pretend)") | ||
# BUG(?): Should be ExportData._inside_rms ? | ||
self._inside_rms = True # type: ignore[misc] | ||
ExportData._inside_rms = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the below ex.
from dataclasses import dataclass
from typing import ClassVar
@dataclass
class Bar:
x: ClassVar[str] = "unset"
b = Bar()
print(b, b.x)
b.x = "set"
print(Bar.x, b.x)
Bar.x = "set"
print(Bar.x, b.x)
>>>
Bar() unset
unset set
set set
If we update the instance reference of x, it will only be updated for that instance of ExportData.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot see any situation where we would like to set this on one instance of ExportData only in the context of the same script. That said, I believe this is for testing purposes only, to emulate fmu-dataio being ran inside the RMS environment. Fixing this sounds like a rational thing to do, but this also feels like an issue more than a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JB 👍 . Yes these are used for testing only, but correct syntax is important still.
@@ -34,11 +34,10 @@ | |||
|
|||
INPUT_FOLDER = Path("../output/maps/grid_averages") | |||
|
|||
dataio.ExportData._inside_rms = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume people do not import this example into there prod. code. But who knows, feels sketchy to set this flag on import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is emulating being ran inside RMS, I believe. So this should never be used like this in production code. I don't think the likelihood of that happening is particularly significant but Murphys's Law tells us it will happen. Not sure what other options there are, but perhaps instead of setting this we could manipulate the environment so that fmu-dataio believes it is inside RMS instead.
@@ -763,11 +765,16 @@ def _establish_pwd_rootpath(self) -> None: | |||
logger.info("The casepath is hard set as %s", self._rootpath) | |||
|
|||
else: | |||
if self._inside_rms or INSIDE_RMS or "RUN_DATAIO_EXAMPLES" in os.environ: | |||
if ExportData._inside_rms or INSIDE_RMS or DATAIO_EXAMPLES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally a bit weird that we we set the flag to true, if it is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.