Complete mypy type coverage for entire codebase #403

janbjorge · 2024-01-08T14:20:54Z

Merge after: #405

janbjorge · 2024-01-08T23:50:04Z

src/fmu/dataio/dataio.py

    subfolder: str = ""
    tagname: str = ""
-    timedata: Optional[List[list]] = None
+    timedata: List[List] | None = None


These must be typing.List due to how _validate_variable(...) is built. Can be converted to list when that functions is cleaned up.

mferrera

Super nice to see the full code base get fully typed like, right away. Wow. I tried to speedrun this review to give some feedback before end of day so I mean no offense with any low quality comments 😄

I'm going to defer approving more substantive PRs since I think @perolavsvendsen and @jcrivenaes should probably be doing that until we're more formally in it.

.github/workflows/mypy.yml

src/fmu/dataio/_design_kw.py

src/fmu/dataio/_filedata_provider.py

src/fmu/dataio/_objectdata_provider.py

src/fmu/dataio/_utils.py

src/fmu/dataio/dataio.py

mferrera · 2024-01-09T14:36:07Z

src/fmu/dataio/dataio.py

+        toutfile, md5 = export_file_compute_checksum_md5(
+            obj,
+            outfile,
+            outfile.suffix,
+            flag=useflag,  # type: ignore
+            # BUG(?): Looks buggy, if flag is bool export_file will blow up.


Might be worth making an issue over this checksum function, perhaps a few things that could be improved based upon your comments in here

Yeah, i have a few other potential bugs that need tickets as well.

janbjorge · 2024-01-10T08:45:18Z

src/fmu/dataio/_oyaml.py

@@ -1,4 +1,5 @@
 # flake8: noqa
+# type: ignore


I think this file can be simplified a bit, looks like there is some relics of py27 in here.

True, is was borrowed from another library in the python 3.4 days

src/fmu/dataio/dataio.py

perolavsvendsen · 2024-01-10T09:38:40Z

src/fmu/dataio/dataio.py

@@ -761,8 +766,8 @@ def _establish_pwd_rootpath(self):
            if self._inside_rms or INSIDE_RMS or "RUN_DATAIO_EXAMPLES" in os.environ:
                self._rootpath = (self._pwd / "../../.").absolute().resolve()
                logger.info("Run from inside RMS (or pretend)")
-                self._inside_rms = True
-        # make some extra keys in settings:
+                # BUG(?): Should be ExportData._inside_rms ?


@jcrivenaes 👀

Yes a class variable initially; but I think the instance variable will just behave like (i.e. borrow from) the class variable?

Im not sure, i have todo some research. Will followup on this in another PR/issue. My gut tells me this works due what Jan wrote above. Its just a bit counterintuitive due to it being a ClassVar.

perolavsvendsen

As far as I can see, no (expected) impact on runtime. Nice clean up. See comments, also would be nice to have opinions from @jcrivenaes before merging. But from my perspective: 🚀

.github/workflows/ci-fmudataio.yml

mferrera

LGTM, very nice. You might consider leaving notes about potential bugs as issues rather than comments

mferrera · 2024-01-10T11:58:08Z

src/fmu/dataio/_utils.py

    """Export and compute checksum, with possibility to use a tmp file."""

-    usefile = filename
+    usefile: Path | None = filename


if tmp: tmpdir = tempfile.TemporaryDirectory() filename = Path(tmpdir.name) / "tmpfile" export_file(obj, filename, extension, flag=flag) checksum = md5sum(filename) if tmp: tmpdir.cleanup() return filename if not tmp else None, checksum

Just an idea, helps to keep the typing less optional

Good input, but i wonder this functions should take in buffer or something. Creating a file here seems a bit odd to me.

jcrivenaes

Nice work; seems that you also have uncovered some bugs (which is probably one achievement from doing type annotations)

jcrivenaes · 2024-01-10T11:59:29Z

src/fmu/dataio/_utils.py

-    if res is not None:
-        return res
+    with contextlib.suppress(ValueError):
+        return float(value)

    return value


Will we ever reach this line? (unclear for me)

Int from string conversion will fail if the value contains a . or is "-inf/inf/nan".

Ex.

>>> float("nan") nan >>> int("nan") Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: invalid literal for int() with base 10: 'nan' >>>

jcrivenaes · 2024-01-10T12:00:34Z

src/fmu/dataio/_utils.py

@@ -492,7 +497,7 @@ def parse_timedata(datablock: dict, isoformat=True):
    if isinstance(datablock["time"], list):
        date0 = datablock["time"][0]["value"]

-        if len(datablock["time"] == 2):
+        if len(datablock["time"]) == 2:


OBS thanks for fixing this bug

Sneaky, but all thanks to mypy.

jcrivenaes · 2024-01-10T12:05:06Z

src/fmu/dataio/dataio.py

@@ -761,8 +766,8 @@ def _establish_pwd_rootpath(self):
            if self._inside_rms or INSIDE_RMS or "RUN_DATAIO_EXAMPLES" in os.environ:
                self._rootpath = (self._pwd / "../../.").absolute().resolve()
                logger.info("Run from inside RMS (or pretend)")
-                self._inside_rms = True
-        # make some extra keys in settings:
+                # BUG(?): Should be ExportData._inside_rms ?


Yes a class variable initially; but I think the instance variable will just behave like (i.e. borrow from) the class variable?

.DS_Store

janbjorge commented Jan 8, 2024

View reviewed changes

janbjorge changed the title ~~Fully typed and pyupgrade --py3-plus~~ Fully typed Jan 8, 2024

janbjorge self-assigned this Jan 9, 2024

janbjorge mentioned this pull request Jan 9, 2024

Add security md #406

Merged

janbjorge added the enhancement New feature or request label Jan 9, 2024

janbjorge marked this pull request as ready for review January 9, 2024 12:09

Fully typed fmu-dataio

2e1eb9b

janbjorge requested review from mferrera, jcrivenaes and tnatt January 9, 2024 13:48

mferrera reviewed Jan 9, 2024

View reviewed changes

janbjorge requested a review from mferrera January 10, 2024 08:36

janbjorge commented Jan 10, 2024

View reviewed changes

janbjorge requested a review from perolavsvendsen January 10, 2024 08:54

perolavsvendsen reviewed Jan 10, 2024

View reviewed changes

src/fmu/dataio/dataio.py Outdated Show resolved Hide resolved

perolavsvendsen reviewed Jan 10, 2024

View reviewed changes

perolavsvendsen mentioned this pull request Jan 10, 2024

checksum function, perhaps a few things that could be improved #410

Closed

perolavsvendsen approved these changes Jan 10, 2024

View reviewed changes

jcrivenaes reviewed Jan 10, 2024

View reviewed changes

.github/workflows/ci-fmudataio.yml Outdated Show resolved Hide resolved

mferrera approved these changes Jan 10, 2024

View reviewed changes

jcrivenaes approved these changes Jan 10, 2024

View reviewed changes

Review comments

985ceea

janbjorge changed the title ~~Fully typed~~ Complete mypy type coverage for entire codebase Jan 10, 2024

janbjorge merged commit cafb98f into equinor:main Jan 10, 2024
11 checks passed

janbjorge deleted the add-mypy branch January 10, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete mypy type coverage for entire codebase #403

Complete mypy type coverage for entire codebase #403

janbjorge commented Jan 8, 2024 •

edited

Loading

janbjorge Jan 8, 2024

mferrera left a comment

mferrera Jan 9, 2024

janbjorge Jan 9, 2024

perolavsvendsen Jan 10, 2024

janbjorge Jan 10, 2024

jcrivenaes Jan 10, 2024

perolavsvendsen Jan 10, 2024

jcrivenaes Jan 10, 2024

janbjorge Jan 10, 2024 •

edited

Loading

perolavsvendsen left a comment

mferrera left a comment

mferrera Jan 10, 2024

janbjorge Jan 10, 2024

jcrivenaes left a comment

jcrivenaes Jan 10, 2024

janbjorge Jan 10, 2024

jcrivenaes Jan 10, 2024

janbjorge Jan 10, 2024

jcrivenaes Jan 10, 2024

Complete mypy type coverage for entire codebase #403

Complete mypy type coverage for entire codebase #403

Conversation

janbjorge commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

mferrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbjorge Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

perolavsvendsen left a comment

Choose a reason for hiding this comment

mferrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrivenaes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbjorge commented Jan 8, 2024 •

edited

Loading

janbjorge Jan 10, 2024 •

edited

Loading