Generalized macOS stdout handler in `Config` by AlexPatrie · Pull Request #137 · biosimulators/Biosimulators_utils

AlexPatrie · 2023-05-08T17:43:04Z

What new features does this PR implement?
Please summarize the features that this PR implements. As relevant, please indicate the issues that the PR closes.

Adds Optional default setting for StandardOutputCapturerLevel to Config object through a validation block. Adjusted default setting from "c" to "python" (resolves dependency on capturer #135)
Adds type annotations to functions

What bugs does this PR fix?
Please summarize the bugs that this PR fixes. As relevant, please indicate the issues that the PR closes.

Fixes frozen stdout and no-output issues like biosimulators no longer working on macOS Biosimulators_COPASI#56

How have you tested this PR?
Pytest, and self testing.

eagmon · 2023-05-09T19:24:03Z

biosimulators_utils/config.py


+CURRENT_PLATFORM = platform.system()
+try:
+    assert CURRENT_PLATFORM == "Darwin"


so this will catch all Mac OS? Have you also tested the bug in non-Mac OS? Why is it that python is connected to Mac and all others use c?

eagmon · 2023-05-09T19:24:47Z

biosimulators_utils/config.py

                simulation results of SED documents
            SAVE_PLOT_DATA (:obj:`bool`, optional): whether to save data for plots alongside data for reports in CSV/HDF5 files
-            REPORT_FORMATS (:obj:`list` of :obj:`str`, optional): default formats to generate reports in
+            REPORT_FORMATS (:obj:`list` of :obj:`str`, optional): default formats to generate reports in-->defaults to 'csv'


This PR sets the default to None

I originally was generating the value of REPORT_FORMATS to have a default constructor setting of [ReportFormat.csv] (formats are of type(enum.Enum)), or even [ReportFormat.h5]. Our good friend SonarCloud complained about this definition, which makes sense as the definition is also performing a very basic operation other than just instantiation. This doc string annotation was from when I had it in the definition and must have missed it.

After review, that doc string is actually as I intended it to be. Following the logic of the constructor, if there is no value for the REPORT_FORMATS attribute upon instantiation, it defaults to 'csv'. As per the constructor: self.REPORT_FORMATS = REPORT_FORMATS or [ReportFormat.csv], which evaluates roughly as:

if not REPORT_FORMATS:
REPORT_FORMATS = [ReportFormat.csv]

eagmon · 2023-05-09T19:26:25Z

biosimulators_utils/config.py

        self.SAVE_PLOT_DATA = SAVE_PLOT_DATA
-        self.REPORT_FORMATS = REPORT_FORMATS
-        self.VIZ_FORMATS = VIZ_FORMATS
+        self.REPORT_FORMATS = REPORT_FORMATS or [ReportFormat.csv]


Hah, I saw this and thought it was an actual csv file rather than a csv attribute of ReportFormat.

You know, I did at first as well! As I mentioned, I moved the logic for this inside the actual constructor as per SonarCloud.

eagmon · 2023-05-09T19:28:28Z

biosimulators_utils/config.py

-                 DEBUG=False):
+                 DEBUG=False,
+                 STDOUT_LEVEL=DEFAULT_STDOUT_LEVEL,
+                 **CUSTOM_SETTINGS):


using the ** I see. kind of dangerous that anything can come in here and will be applied to the settings. Do you have examples of what kind of kwargs might be used?

This was a part of my original implementation in an idea that I had. As you can see starting at line 181, these **kwargs become unpacked in the private method __getcustomsettings() and returned as a dict in the attribute CUSTOM_SETTINGS. I realize the danger of arbitrarily using unpacking, but I have funneled it into a collection.

Devising some presentable use cases now to plead my case, hah.

eagmon · 2023-05-09T19:30:03Z

I left some comments, this is looking pretty close. What's failing in the CI?

sonarqubecloud · 2023-05-09T21:54:33Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

AlexPatrie · 2023-05-10T02:22:00Z

I left some comments, this is looking pretty close. What's failing in the CI?

I just need to take care of some linting errors in this case.

AlexPatrie

I am going to add an example use case for the unpacking **CUSTOM_SETTINGS.

AlexPatrie · 2023-05-10T02:39:40Z

biosimulators_utils/config.py

-                 DEBUG=False):
+                 DEBUG=False,
+                 STDOUT_LEVEL=DEFAULT_STDOUT_LEVEL,
+                 **CUSTOM_SETTINGS):


This was a part of my original implementation in an idea that I had. As you can see starting at line 181, these **kwargs become unpacked in the private method __getcustomsettings() and returned as a dict in the attribute CUSTOM_SETTINGS. I realize the danger of arbitrarily using unpacking, but I have funneled it into a collection.

AlexPatrie · 2023-05-10T12:12:00Z

biosimulators_utils/config.py

-                 DEBUG=False):
+                 DEBUG=False,
+                 STDOUT_LEVEL=DEFAULT_STDOUT_LEVEL,
+                 **CUSTOM_SETTINGS):


Devising some presentable use cases now to plead my case, hah.

AlexPatrie · 2023-05-10T12:20:45Z

biosimulators_utils/config.py

                simulation results of SED documents
            SAVE_PLOT_DATA (:obj:`bool`, optional): whether to save data for plots alongside data for reports in CSV/HDF5 files
-            REPORT_FORMATS (:obj:`list` of :obj:`str`, optional): default formats to generate reports in
+            REPORT_FORMATS (:obj:`list` of :obj:`str`, optional): default formats to generate reports in-->defaults to 'csv'


I originally was generating the value of REPORT_FORMATS to have a default constructor setting of [ReportFormat.csv] (formats are of type(enum.Enum)), or even [ReportFormat.h5]. Our good friend SonarCloud complained about this definition, which makes sense as the definition is also performing a very basic operation other than just instantiation. This doc string annotation was from when I had it in the definition and must have missed it.

AlexPatrie · 2023-05-10T12:22:10Z

biosimulators_utils/config.py

        self.SAVE_PLOT_DATA = SAVE_PLOT_DATA
-        self.REPORT_FORMATS = REPORT_FORMATS
-        self.VIZ_FORMATS = VIZ_FORMATS
+        self.REPORT_FORMATS = REPORT_FORMATS or [ReportFormat.csv]


You know, I did at first as well! As I mentioned, I moved the logic for this inside the actual constructor as per SonarCloud.

AlexPatrie added 4 commits May 8, 2023 13:37

implemented generalized macOS handler in Config

85c0f2d

added custom settings kwargs and optional format args

5250394

adjusted minor logic

22cbeff

added lazy evaluation for report and viz formats in Config factory

4d17e75

AlexPatrie requested a review from eagmon May 8, 2023 20:51

AlexPatrie changed the title ~~Generalized macOS handler in Config~~ Generalized macOS handler in Config May 8, 2023

AlexPatrie changed the title ~~Generalized macOS handler in Config~~ Generalized macOS stdout handler in Config May 8, 2023

AlexPatrie requested a review from jcschaff May 8, 2023 21:00

eagmon reviewed May 9, 2023

View reviewed changes

Added acceptable format helpers.

8997605

AlexPatrie commented May 10, 2023

View reviewed changes

Comments

Conversation

AlexPatrie commented May 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eagmon commented May 9, 2023

Uh oh!

sonarqubecloud bot commented May 9, 2023

Uh oh!

AlexPatrie commented May 10, 2023

Uh oh!

AlexPatrie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants