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.

Sign up for GitHub

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

Jump to bottom

BUG: Use classvar when setting inside-rms flag #414

Merged
merged 1 commit into from
Jan 12, 2024
Merged

BUG: Use classvar when setting inside-rms flag #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions examples/s/d/nn/xcase/realization-0/iter-0/rms/bin/export_propmaps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,10 @@

INPUT_FOLDER = Path("../output/maps/grid_averages")

dataio.ExportData._inside_rms = True
Copy link
Contributor Author

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.

Copy link
Member

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.



def main():
"""Exporting maps from clipboard"""
dataio.ExportData._inside_rms = True

files = INPUT_FOLDER.glob("*.gri")

Expand Down
5 changes: 5 additions & 0 deletions src/fmu/dataio/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ def detect_inside_rms() -> bool:
return inside_rms


def dataio_examples() -> bool:
# This flag is set when the `run-exmaples.sh` script runs.
return "RUN_DATAIO_EXAMPLES" in os.environ


def drop_nones(dinput: dict) -> dict:
"""Recursively drop Nones in dict dinput and return a new dict."""
# https://stackoverflow.com/a/65379092
Expand Down
17 changes: 12 additions & 5 deletions src/fmu/dataio/dataio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
)
from ._utils import (
create_symlink,
dataio_examples,
detect_inside_rms,
drop_nones,
export_file_compute_checksum_md5,
Expand All @@ -37,6 +38,7 @@
uuid_from_string,
)

DATAIO_EXAMPLES: Final = dataio_examples()
INSIDE_RMS: Final = detect_inside_rms()


Expand Down Expand Up @@ -748,7 +750,7 @@ def _establish_pwd_rootpath(self) -> None:
"""
logger.info(
"Establish pwd and actual casepath, inside RMS flag is %s (actual: %s))",
self._inside_rms,
ExportData._inside_rms,
INSIDE_RMS,
)
self._pwd = Path().absolute()
Expand All @@ -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:
Copy link
Contributor Author

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?

logger.info(
"Run from inside RMS: ExportData._inside_rms=%s, "
"INSIDE_RMS=%s, DATAIO_EXAMPLES=%s",
ExportData._inside_rms,
INSIDE_RMS,
DATAIO_EXAMPLES,
)
self._rootpath = (self._pwd / "../../.").absolute().resolve()
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
Copy link
Contributor Author

@janbjorge janbjorge Jan 11, 2024 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Collaborator

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.

self._usecontext = self.fmu_context # may change later!

logger.info("pwd: %s", str(self._pwd))
Expand Down
Loading