Address all pylint errors in the current codebase #58
Conversation
lbianchi-lbl
force-pushed
the
pylint-false-positives
branch
from
9bec1e7 to
d261c8a
Compare
ksbeattie
added
Priority:High
lbianchi-lbl
force-pushed
the
pylint-false-positives
branch
from
d261c8a to
1840c47
Compare
| @@ -268,6 +268,9 @@ def report(self, time_point=0, dof=False, ostream=None, prefix=""): | |||
| f"Activated Blocks: {nb}") | |||
|
|
|||
| if performance is not None: | |||
| # PYLINT-WHY: pylint has no way of knowing that performance is supposed to be dict-like | |||
| # pylint: disable=unsubscriptable-object | |||
| # PYLINT-TODO: alternatively, have the function return an empty dict and test with `if performance:` | |||
There was a problem hiding this comment.
I do not see a problem with this suggestion (although I cannot guarantee it won't break something somewhere else). I don't think there was a particular reason for having get_performance_contents return None by default.
There was a problem hiding this comment.
A quick check showed at least two unit tests in test_process_base() that would need to be updated. Apart from the (however minor) chance of breaking the existing API, I'm not sure I agree with pylint that this is an error: personally, for this and similar cases, I don't think there's anything particularly wrong with returning None as a sentinel value.
Seeing that all errors of the same type are related to the use of the same _get_performance_contents() in derived classes, I would propose to add this specific error (E1128: Assigning result of a function call, where the function returns None (assignment-from-none)) to the list of globally ignored checks, and leave the similar but semantically quite different E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return) active, as IMO it covers a case that is much more likely to be accidental ("forgetting" to return a value where the client expects some, versus explicitly using return None).
| **kwargs, | ||
| default=default, | ||
| initialize=initialize) | ||
| return self.state_block_class( # pylint: disable=not-callable |
There was a problem hiding this comment.
For this case, I don't see any reason why we could not make state_block_class a method rather than a property - that way pylint should be able to tell that it is callable.
There was a problem hiding this comment.
One consequence that this change would have is that clients of state_block_class would have to use it as a callable (state_block_class()), i.e. breaking the current API. This would not only require changing all call sites (about 20 from a quick search), but also having to use the (IMO slightly awkward) double-parens syntax to call the class itself (e.g. state_block = my_block.state_block_class()(**state_block_params)).
Taken these into account, and considering that I don't think there's anything wrong with the current solution of using a callable through a property (a reasonable strategy to effectively "build" an instance-specific method at runtime), I can see the argument for leaving the pylint directive and avoid changing the code.
| __all__ = [ | ||
| 'ReactionBlock', # pylint: disable=undefined-all-variable | ||
| 'ReactionParameterBlock' # pylint: disable=undefined-all-variable | ||
| ] |
There was a problem hiding this comment.
@eslickj Do we even actually use this __all__ attribute?
| @@ -224,6 +224,10 @@ def mixer_pressure_constraint(b, t): | |||
| @m.fs.hotwell.Constraint(m.fs.time) | |||
| def mixer_pressure_constraint(b, t): | |||
| return b.condensate_state[t].pressure == b.mixed_state[t].pressure | |||
| # PYLINT-TODO check if the "function-redefined" pylint error can be addressed | |||
There was a problem hiding this comment.
This is a tricky one. Pylint is correctly identifying that the methods are being redefined, although that does not affect code functionality as they are being used in different namespaces (which pylint cannot easily see). This could easily be fixed by changing the name of the method (e.g. appending a prefix or suffix for the unit in question), but we would need to make sure that other parts of the code are not looking for these constraints by name.
Alternatively, using call syntax would work fine, and avoid any issues with changing the name of the constraint. The rules are sufficiently different to preclude writing a common method for all, but each constraint could define a new method with a unique name and not affect the code.
There was a problem hiding this comment.
Short answer - I suggest refactoring the code, which should not be hard.
There was a problem hiding this comment.
Thanks for the explanation, it was useful for me to understand the context a bit better. On second thought, my personal preference would be to add the pylint directive (leaving the code as it is) rather than making changes to the code:
- I initially thought that the functionality (i.e. the inner function's body) was identical in all cases, but as you point out, this is not the case
- This particular portion of the code doesn't seem to be covered by unit tests (might be by component/integration tests)
- I peeked at the source code for
Constraintinpyomo.core.base.constraintand, unless the implementation used in this part of the IDAES code is significantly simpler, I think I've seen enoughdark magicmetaprogramming to make me hesitant to jump in and make the changes without more research oramuletstests - In general, I would argue that the type of error that pylint is flagging here does not apply in this particular situation, as the decorator is applied directly to the function definition; therefore, I'd lean more toward considering this a false positive
There was a problem hiding this comment.
I agree that it is a false positive, but I could also see it confusing someone reading the code as well. For me, I am somewhat hesitant to add pylint directive unless absolutely necessary, in case this ends up masking real problems in the future.
That said, this is not something I would ask you to fix - if we were going to change the model code I would ask the powerplant team to do it.
|
Thanks a lot @andrewlee94, this is exactly the type of review feedback I was looking for! I'll address your comments and reply or confirm the proposed fix as appropriate. |
Codecov Report
@@ Coverage Diff @@
## main #58 +/- ##
========================================
Coverage 63.78% 63.78%
========================================
Files 312 312
Lines 50261 50467 +206
Branches 8860 8918 +58
========================================
+ Hits 32057 32192 +135
- Misses 16640 16692 +52
- Partials 1564 1583 +19
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## main #58 +/- ##
==========================================
- Coverage 64.62% 64.62% -0.01%
==========================================
Files 334 334
Lines 53767 53775 +8
Branches 9454 9454
==========================================
+ Hits 34747 34752 +5
- Misses 17282 17286 +4
+ Partials 1738 1737 -1
Continue to review full report at Codecov.
|
lbianchi-lbl
changed the title
lbianchi-lbl
requested review from
eslickj,
andrewlee94,
MAZamarripa,
mengleCMU and
Robbybp
Thanks, that's perfect. I think it's best if I take care of these small changes directly here so that we don't create too many extra PRs. |
There was a problem hiding this comment.
Re: reaction_base, line 536, the proposed fix is correct. The second instance of attr is probably a holdover from an earlier version.
Re: pressure_changer, line 620: This looks like it might be the hanging , at the end of each line. The .format() itself looks right, but for some reason there is a comma at the end of each line which is breaking up the strings.
Re: supercritical_steam_cycle: My personal response would be to give the methods unique names as I think that would be better practice and avoid any potential confusion by users. However, the power plant teams should have the final call on this (@eslickj, @MAZamarripa).
| @@ -0,0 +1,173 @@ | |||
| """ | |||
There was a problem hiding this comment.
Do we need to add the copyright header to this (and check any other new files too)?
There was a problem hiding this comment.
Good point, I'm adding it to the dev call agenda.
Otherwise, pylint exiting with a nonzero exit code would prevent further steps from executing (there is no `if: failure()` equivalent for composite steps actions)
lbianchi-lbl
force-pushed
the
pylint-false-positives
branch
from
af207df to
5726777
Compare
Summary/Motivation
Get rid of all pylint errors to obtain a "clean slate" so that pylint can be activated as a required check for future PRs.
Strategy
This PR implement changes aimed to address everything that pylint reports as an error in different ways, depending if the reported error is a false positive, i.e. a statement that pylint considers a cause of runtime error, but that doesn't actually cause errors at runtime, or a true positive, i.e. a statement that would (or does) result in a runtime error if that portion of the code is run:
PYLINT-TODOso that it's easy to find these fixes within the codebaseChanges proposed in this PR
.pylinttop-level directory to store all pylint-related code and configurationpylintrcfileReviewing
Remaining errors are listed in the checkist below. There is a link to each affected file in the header, and links pointing to the specific line where the error was detected (using the GitHub/blame/URL endpoint to display the edit history of the affected line)I'll be pinging the author(s) of the affected code (as indicated bygit blameand/or previous conversations) both by requesting a PR review and by @-mentioning them pointing to specific lines; of course, everyone is more than welcome to chime in and clarifyThe same applies to errors with an unchecked checkbox where I haven't yet mentioned anyone (either for lack of a clear author, because I forgot, etc)In general, I expect that a number of these errors to be in parts of the code that are rarely or not used anymore (which is why they haven't been fixed until now); if this is the case, let me know if it makes sense for me to try to address the pylint errors at all or they can be skipped (because e.g. the files will be removed or deprecated soon)Errors left to fix (list compiled on 2021-05-07, redundant as of 2021-05-21)
# PYLINT-TODOsentinel comments; see Address all pylint errors in the current codebase #58 (comment) for detailsClick to display the original checklist
idaes.core.reaction_baseReactionBlockDataBase.__getattr__: (link) (E1305)Too many arguments for format string(too-many-format-args)@andrewlee94: the error comes from the format string having two "slots" but three arguments being used:.format(self.name, attr, attr). Would it be OK to change that to.format(self.name, attr)?idaes.core.util.dyn_utilscopy_non_time_indexed_values: (link) (E0602)Undefined variable 'varname'(undefined-variable)@Robbybp what should be used here instead of the non-existingvarname?idaes.core.util.unit_costingpressure_changer_costing: (link) (E1305)Too many arguments for format string(too-many-format-args)pressure_changer_costing.base_pump_rule: (link) (E1305)Too many arguments for format string(too-many-format-args)pressure_changer_costing: (link) (E1305)Too many arguments for format string(too-many-format-args)pressure_changer_costing: (link) (E1305)Too many arguments for format string(too-many-format-args)pressure_changer_costing: (link) (E1305)Too many arguments for format string(too-many-format-args)platforms_ladders.CPL_rule: (link) (E1305)Too many arguments for format string(too-many-format-args)platforms_ladders: (link) (E1305)Too many arguments for format string(too-many-format-args)fired_heater_costing.CB_rule: (link) (E1305)Too many arguments for format string(too-many-format-args)idaes.core.util.convergence.convergence_baseStats.to_dmf: (link) (E0602)Undefined variable 'stats'(undefined-variable)statsdoesn't exist, but this is a method of theStatsclass. Shouldstatsbeselfinstead?idaes.core.util.convergence.mpi_utilsParallelTaskManager.allgather_global_data: (link) (E0602)Undefined variable 'global_data_list_of_lists'(undefined-variable)idaes.commands.baseE1120)No value for argument 'verbose' in function call(no-value-for-parameter)E1120)No value for argument 'quiet' in function call(no-value-for-parameter)idaes.generic_models.unit_models.pressure_changerPressureChangerData.model_check: (link) (E1305)Too many arguments for format string(too-many-format-args)idaes.power_generation.flowsheets.supercritical_steam_cycle.supercritical_steam_cyclecreate_model.mixer_pressure_constraint: (link) (E0102)function already defined line 182(function-redefined)create_model.mixer_pressure_constraint: (link) (E0102)function already defined line 182(function-redefined)create_model.mixer_pressure_constraint: (link) (E0102)function already defined line 182(function-redefined)idaes.surrogate.mainPysmo_rbf.handle_results: (link) (E1101)Instance of 'RadialBasisFunctions' has no 'rbf_generate_expression' member(no-member)Pysmo_kriging.handle_results: (link) (E1101)Instance of 'KrigingModel' has no 'kriging_generate_expression' member(no-member)idaes.surrogate.ripeE0603)Undefined variable name 'massact' in __all__(undefined-all-variable)idaes.surrogate.ripe.writewriteripe: (link) (E0401)Unable to import 'ripe'(import-error)writemodel: (link) (E0401)Unable to import 'ripe'(import-error)writemodel: (link) (E0401)Unable to import 'alamopy'(import-error)writeminlp: (link) (E0401)Unable to import 'ripe'(import-error)idaes.surrogate.helmet.HelmetviewMultResults: (link) (E1101)Module 'idaes.surrogate.helmet.Plotting' has no 'multSSECombo' member(no-member)idaes.surrogate.helmet.GAMSDataWriteSNDdt: (link) (E1306)Not enough arguments for format string(too-few-format-args)SNDdt: (link) (E1306)Not enough arguments for format string(too-few-format-args)SNDdt: (link) (E1306)Not enough arguments for format string(too-few-format-args)idaes.surrogate.alamopy.doalamogetTrainingData: (link) (E1102)almerror is not callable(not-callable)getTrainingData: (link) (E1102)almerror is not callable(not-callable)addBasisConstraints: (link) (E0602)Undefined variable 'group_constraints'(undefined-variable)addBasisConstraints: (link) (E0602)Undefined variable 'basis_constraints'(undefined-variable)parseKwargs: (link) (E1102)almerror is not callable(not-callable)readTraceFile: (link) (E0602)Undefined variable 'writethis'(undefined-variable)_construct_mock: (link) (E0602)Undefined variable 'debug'(undefined-variable)idaes.surrogate.alamopy.allcardallcard: (link) (E0401)Unable to import 'alamopy.writethis'(import-error)Original Summary/Motivation (superseded and only partially relevant as of 2021-05-07)
This PR implements changes aimed at getting rid of false-positives errors, i.e. portions of the code that do not cause runtime errors, but are erroneously flagged as such by pylint because of limitations in pylint's static analysis.
In particular, it introduces a pylint transform plugin that makes pylint aware of
ProcessBlocksubclasses that are dynamically generated at runtime. This eliminates the pylint errors described in IDAES/idaes-dev#1159 without requiring any changes to the code.This is part of the efforts towards IDAES/idaes-dev#1149. Specifically, this PR contains only changes that affect pylint exclusively, and thus do not alter the runtime behavior of the code in any way, i.e.:
The idea is to keep those sets of changes that do affect the code itself (and that therefore could alter the runtime behavior) in dedicated PRs, to streamline both the fixes and the review process. In other words, reviews for this PR can concentrate entirely on the pylint analysis, without having to take into account compatibility, introducing regressions, or other code-specific considerations.
In addition, these changes aggregate most of the configuration into the pylint RC file located at .pylint/pylintrc, allowing to share the same settings among different use cases in addition to CI checks, e.g. direct invocation, and integration with IDEs/editors supporting static analysis with pylint.
Changes proposed in this PR
ProcessBlocksubclasses false positivesLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: