Update pylint version to match pyomo (v 3.3.9) #1684

sufikaur · 2025-11-10T19:28:37Z

Fixes/Resolves:

Issue 1679
See idaes PR 1683

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

…lint-upgrade

sufikaur · 2025-11-18T01:31:45Z

After looking at each test, I think that the warnings can be disabled globally.
There are 2 tests that I would like others to confirm will not run into issues.

Currently, I have disabled on each line, but after reviewing, they can be generalized

sufikaur · 2025-11-18T01:42:23Z

watertap/unit_models/zero_order/coag_and_floc_zo.py

            return blk.chemical_flow_mass[t, j] == pyunits.convert(
-                chemical_dosage * blk.properties[t].flow_vol,
+                chemical_dosage
+                * blk.properties[t].flow_vol,  # don't fully understand this test


I would appreciate someone else looking at this test. @bknueven do you recall this model?

This is fine, if clunky.

That said, I think in many, many places, instead of suppressing pylint we should instead add an additional else clause which raises an exception. WaterTAP is big enough that we should have our own exceptions anyways.

E.g., in this case the else clause could raise a WaterTAPDeveloperError, or if we want to be cute, a FronzenPipes error. It doesn't even need a message if we know we'll never hit the else clause upon inspection.

Thank you for taking a look and sounds good-- suppressing was a temporary step and throwing a dev error makes sense.

bknueven · 2025-11-19T02:12:08Z

watertap/tools/oli_api/util/watertap_to_oli_helper_functions.py

+    molecule = ""
    if len(components) == 1:
        molecule = components[0]
    elif len(components) == 2:


I think this is another case where we should raise an exception if neither condition is satisfied.

requirements-dev.txt

watertap/core/util/chemistry.py

MarcusHolly · 2025-11-21T21:02:42Z

watertap/core/util/initialization.py

+        # can fix by initizalizing msg or can disable message
+        msg = "Message failed to initialize."


I would probably just replace the else clause with a generic WaterTAPDeveloperError because the issue is that the else clause is calling a message that hasn't necessarily been initialized yet, right?

I remember there was a reason we did a msg here and not an error or log ... I think

MarcusHolly · 2025-11-21T21:05:18Z

watertap/costing/unit_models/gac.py

+    # contactor_cost_coeff_data, other_cost_param_data, energy_consumption_coeff_data = (
+    #     {},
+    #     {},
+    #     {},
+    # )  # other option is to initialize as empty dicts and fill based on contactor type


I think this solution is fine

Just confirming, this solution meaning the initializing empty dicts

MarcusHolly · 2025-11-22T02:25:48Z

watertap/unit_models/uv_aop.py

+        # take a look at this test together
+        iscale.set_scaling_factor(
+            self.uv_intensity, sf  # pylint: disable=used-before-assignment
+        )


Not sure what you mean by this comment

Mostly referring to the comment about the if statement below. Needed more eyes in particular on this test

MarcusHolly · 2025-11-22T02:26:19Z

watertap/unit_models/uv_aop.py

+            iscale.set_scaling_factor(
+                self.exposure_time, sf
+            )  # why is this one set inside the if but none of the rest?


Good catch - this should not be inside the if statement

Shouldn't all be inside the if statement? I think the fact that they are outside is causing the sf possibly used before assignment error

I simply disabled for now since the comment above says that an error would be thrown elsewhere

requirements-dev.txt

sufikaur · 2025-11-26T20:53:32Z

watertap/costing/unit_models/gac.py

+    # contactor_cost_coeff_data, other_cost_param_data, energy_consumption_coeff_data = (
+    #     {},
+    #     {},
+    #     {},
+    # )  # other option is to initialize as empty dicts and fill based on contactor type


Just confirming, this solution meaning the initializing empty dicts

watertap/property_models/multicomp_aq_sol_prop_pack.py

sufikaur · 2025-11-26T20:59:13Z

watertap/unit_models/uv_aop.py

+        # take a look at this test together
+        iscale.set_scaling_factor(
+            self.uv_intensity, sf  # pylint: disable=used-before-assignment
+        )


Mostly referring to the comment about the if statement below. Needed more eyes in particular on this test

sufikaur · 2025-11-26T21:05:41Z

watertap/unit_models/uv_aop.py

+            iscale.set_scaling_factor(
+                self.exposure_time, sf
+            )  # why is this one set inside the if but none of the rest?


Shouldn't all be inside the if statement? I think the fact that they are outside is causing the sf possibly used before assignment error

I simply disabled for now since the comment above says that an error would be thrown elsewhere

…lint-upgrade

watertap/custom_exceptions.py

bknueven

Thanks @sufikaur. I found a few more small things, but I think this is about ready.

bknueven · 2025-12-02T03:05:15Z

watertap/unit_models/uv_aop.py

            )
-            iscale.set_scaling_factor(
-                self.exposure_time, sf
-            )  # why is this one set inside the if but none of the rest?


I think the answer here is this one is correct, and the other are wrong. The code doesn't make sense otherwise, right?

bknueven · 2025-12-02T03:08:09Z

watertap/unit_models/uv_aop.py

            )
-            iscale.set_scaling_factor(self.exposure_time, sf)
-
+        iscale.set_scaling_factor(self.exposure_time, sf)


Just to reiterate -- I think all the calls to set_scaling_factor in this method should be inside the associated if statement. That's on the only way this code makes sense.

bknueven · 2025-12-02T03:09:18Z

watertap/costing/unit_models/gac.py

+    contactor_cost_coeff_data, other_cost_param_data, energy_consumption_coeff_data = (
+        {},
+        {},
+        {},
+    )


I think this is another one of those times we should raise an exception in an else. Otherwise we will get an error elsewhere and it'll be difficult to trace back here.

changed this to an else

watertap/property_models/water_prop_pack.py

Co-authored-by: MarcusHolly <[email protected]>

kurbansitterley · 2025-11-22T22:21:29Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

        # touch this var since it is required for this method
        self.conc_mol_phase_comp
-
+        """this could also be defined 'state_var["Liq", adjust_by_ion].fix(ion_adjusted)' 


what is this doc string / comment???

kurbansitterley · 2025-11-22T22:27:31Z

watertap/costing/unit_models/ion_exchange.py

    elif ix_type == "anion":
        resin_cost = ion_exchange_params.anion_exchange_resin_cost
+    else:
+        raise WaterTapDeveloperError(


This gets checked in the unit model directly via checking of the charge on a component. It should be essentially impossible to get to this point.

... but maybe that is the point of a developer error?

Yes. If somebody changes something upstream which breaks that assumption, we'll catch it here instead of just charging forward.

kurbansitterley · 2025-11-22T22:30:05Z

watertap/core/util/initialization.py

+        # can fix by initizalizing msg or can disable message
+        msg = "Message failed to initialize."


I remember there was a reason we did a msg here and not an error or log ... I think

Sufi Kaur and others added 2 commits November 10, 2025 11:26

update pylint version to match pyomo

9a91023

Merge branch 'main' into pylint-upgrade

f153a67

sufikaur mentioned this pull request Nov 10, 2025

Update pylint version to match pyomo #1679

Closed

ksbeattie assigned ksbeattie and sufikaur Nov 13, 2025

ksbeattie added the Priority:High High Priority Issue or PR label Nov 13, 2025

Sufi Kaur added 3 commits November 17, 2025 17:24

fix pylint errors

3357562

Merge branch 'main' into pylint-upgrade

9ce9ee8

Merge branch 'pylint-upgrade' of github.com:sufikaur/watertap into py…

f8b3dce

…lint-upgrade

sufikaur requested review from MarcusHolly, adam-a-a, avdudchenko, bknueven, k1nshuk, kurbansitterley and luohezhiming as code owners November 18, 2025 01:30

Sufi Kaur added 2 commits November 17, 2025 17:34

black

2445197

fix black

f755d4e

sufikaur commented Nov 18, 2025

View reviewed changes

Sufi Kaur added 2 commits November 17, 2025 18:13

fix error with extraneous text

c45d295

added custom error and fixed warnings with more confidence :)

21239c9

sufikaur requested a review from ksbeattie as a code owner November 19, 2025 01:57

bknueven reviewed Nov 19, 2025

View reviewed changes

sufikaur requested a review from bknueven November 20, 2025 21:55

MarcusHolly reviewed Nov 22, 2025

View reviewed changes

sufikaur requested a review from MarcusHolly November 26, 2025 20:37

addressed review notes

9320ed4

sufikaur commented Nov 26, 2025

View reviewed changes

Merge branch 'main' into pylint-upgrade

2e0eb07

Sufi Kaur added 2 commits November 26, 2025 14:31

stray letter

ee85064

Merge branch 'pylint-upgrade' of github.com:sufikaur/watertap into py…

8a6227d

…lint-upgrade

MarcusHolly approved these changes Dec 1, 2025

View reviewed changes

watertap/custom_exceptions.py Outdated Show resolved Hide resolved

bknueven reviewed Dec 2, 2025

View reviewed changes

Update watertap/custom_exceptions.py

2ee5ab2

Co-authored-by: MarcusHolly <[email protected]>

kurbansitterley approved these changes Dec 4, 2025

View reviewed changes

sufikaur and others added 8 commits December 4, 2025 16:34

Merge branch 'main' into pylint-upgrade

6352bc0

address review comments

a7b1c21

raise error in gac instead of initializing variables for more clarity

3db22a8

formatting

fdfc75c

remove redundant disable

1f4dd7d

Merge branch 'main' into pylint-upgrade

89bf8af

remove comment

4930e47

Merge branch 'main' into pylint-upgrade

c56dc77

sufikaur requested a review from bknueven December 10, 2025 18:41

bknueven approved these changes Dec 10, 2025

View reviewed changes

ksbeattie merged commit ed5d54f into watertap-org:main Dec 10, 2025
18 checks passed

		# can fix by initizalizing msg or can disable message
		msg = "Message failed to initialize."

Update pylint version to match pyomo (v 3.3.9) #1684

Update pylint version to match pyomo (v 3.3.9) #1684

Uh oh!

Conversation

sufikaur commented Nov 10, 2025

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Uh oh!

sufikaur commented Nov 18, 2025

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!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bknueven 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants