Work on pre-commit hooks, small readme update. #401

yakutovicha · 2022-11-30T09:22:12Z

Remove unused .prospector.yaml
Readme: add --tag-num option to update tag number
Flake8: add more plugins to better analyse the code:
- flake8-bugbear for detecting bugs and potential design problems
- flake8-builtins for verifying that builtins are not used as variable names
- flake8-comprehensions for writing better and consistent comprehensions
- flake8-debugger for checking that there are no forgotten breakpoints
- flake8-logging-format for consistent logging,
- pep8-naming for verifying that PEP8 naming conventions are followed
- pyflakes for checking source files for various errors
- tryceratops to avoid exception anti-patterns

* Remove unused .prospector.yaml * Readme: add --tag-num option to update tag number * Flake8: add more plugins to better analyse the code: * `flake8-bugbear` for detecting bugs and potential design problems * `flake8-builtins` for verifying that builtins are not used as variable names * `flake8-comprehensions` for writing better and consistent comprehensions * `flake8-debugger` for checking that there are no forgotten breakpoints * `flake8-logging-format` for consistent logging, * pep8-naming `for verifying that PEP8 naming conventions are followed` * `pyflakes` for checking source files for various errors * `ryceratops` to avoid exception anti-patterns

yakutovicha · 2022-11-30T10:04:53Z

Hey reviewers 👋

As you can see, I enabled a few flake8 plugins to check the code for various issues, antipatterns, etc. However, there are two types of errors I am not happy with:

TC101 - "Too many try blocks in your function". In principle, I can split them into subfunctions to have one try-except block per function. But I don't know why this is needed. What advantage does it bring?
TC003 - "Avoid specifying long messages outside the exception class". I don't get what it wants from me 😞 . Shortening the string doesn't help with the issue.

My first intention is to disable those checks unless you have a different opinion.

danielhollas · 2022-11-30T13:35:52Z

TC101 - "Too many try blocks in your function". In principle, I can split them into subfunctions to have one try-except block per function. But I don't know why this is needed. What advantage does it bring?
TC003 - "Avoid specifying long messages outside the exception class". I don't get what it wants from me disappointed . Shortening the string doesn't help with the issue.

Here's a detailed description of these rules:
https://github.com/guilatrova/tryceratops/blob/main/docs/violations/TC003.md
https://github.com/guilatrova/tryceratops/blob/main/docs/violations/TC101.md

They both make sense to me, but I don't know where you see these violation so cannot comment if it makes sense to fix or ignore in our context.

danielhollas · 2022-11-30T13:38:15Z

aiidalab_widgets_base/bug_report.py

@@ -111,11 +111,11 @@ def _convert_ansi_codes_to_html(msg):
 def _format_truncated_traceback(traceback, max_num_chars=2000):
    """Truncate the traceback to the given character length."""
    n = 0
-    for i, line in enumerate(reversed(traceback)):
+    for _i, line in enumerate(reversed(traceback)):


I don't understand this change, it looks very confusing to me. Which rule is violated here?

The format checker said something like: "i was not used inside the loop, so add an underscore in front."

Fair, but still looks very confusing to me, never seen a variable (not method) prefixed with underscore. Maybe call it lineno or line_number?

Fair, but still looks very confusing to me, never seen a variable (not method) prefixed with underscore. Maybe call it lineno or line_number?

You meant _lineno/_line_number? Cause I don’t think the name without an underscore will satisfy the checks.

Here are a few words about the use of underscore for the variable names: https://medium.com/python-features/naming-conventions-with-underscores-in-python-791251ac7097

Ah, I misunderstood then. So be it, but I'd still use a more descriptive variable name to make things clearer. Thanks!

In the article they explain the use the use of a leading underscore to indicate private vars / methods, which I was familiar with, but I don't think that is the case here so it still looks odd to me tbh.

You are right. In the end, this is only to satisfy the check. Probably, this part of the code could be written better.

aiidalab_widgets_base/utils/__init__.py

aiidalab_widgets_base/process.py

Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>

yakutovicha · 2022-11-30T19:44:45Z

They both make sense to me, but I don't know where you see these violation so cannot comment if it makes sense to fix or ignore in our context.

Just have a look at the pre-commit ci. They are there.

danielhollas · 2022-12-01T08:52:33Z

As the TC101 indicates, the on_setup_computer() function does look quite busy with three try statements. I don't know how to get rid of the first try, but I'd suggest to combine the second and the third in a way that TC101 suggests:

        try:
            computer = computer_builder.new()
            computer.store()
        except (
            ComputerBuilder.ComputerValidationError,
            common.exceptions.ValidationError,
        ) as err:
            self.message = f"{type(err).__name__}: {err}"
            return False
        except common.exceptions.ValidationError as err:
            self.message = f"Unable to store the computer: {err}."
            return False

The try statements in SmilesWidget.__init__() cannot be combined so I'd suppress the error locally.

EDIT: I just noticed that common.exceptions.ValidationError is shared between the two except clauses. If it is needed in the first one, I guess the try statements cannot be combined. 🤷

danielhollas · 2022-12-01T09:05:13Z

As for TC003, I don't know how to get rid of them since we're using the built-in exception classes. 🤷

Note that the ValueError in viewers.py:791 should probably be a TypeError.

danielhollas · 2022-12-01T09:10:18Z

@yakutovicha I would also suggest to add the nbstripout pre-commit, which automatically strips output cells from Jupyter notebooks. I am using it in my App and find it very useful, and @unkcpz recently added it to QeApp as well.

https://github.com/ispg-group/aiidalab-ispg/blob/00f054b675902f2f5100d36e8858dae76d465257/.pre-commit-config.yaml#L14)

yakutovicha · 2022-12-06T13:55:05Z

@yakutovicha I would also suggest to add the nbstripout pre-commit, which automatically strips output cells from Jupyter notebooks. I am using it in my App and find it very useful, and @unkcpz recently added it to QeApp as well.

https://github.com/ispg-group/aiidalab-ispg/blob/00f054b675902f2f5100d36e8858dae76d465257/.pre-commit-config.yaml#L14)

Thanks, I've just added this.

yakutovicha · 2022-12-06T14:02:33Z

Note that the ValueError in viewers.py:791 should probably be a TypeError.

Thanks, fixed in 87f5f4c

yakutovicha · 2022-12-06T14:13:04Z

As for TC003, I don't know how to get rid of them since we're using the built-in exception classes. 🤷

I think they propose avoiding custom strings in exceptions completely. At least, this is what I understood looking at the link you've shared above.

At the same time, I don't get why the following code receives complaints:

aiidalab-widgets-base/aiidalab_widgets_base/wizard.py

Lines 96 to 98 in 87f5f4c

    
           raise ValueError( 
        
               "The number of steps of a WizardAppWidget must be at least two." 
        
           )

Why the code below doesn't:

aiidalab-widgets-base/aiidalab_widgets_base/viewers.py

Lines 791 to 794 in 87f5f4c

    
           raise TypeError( 
        
               f"Unsupported type {type(structure)}, structure must be one of the following types: " 
        
               "ASE Atoms object, AiiDA CifData or StructureData." 
        
           )

danielhollas

@yakutovicha thanks for the additional changes. I only have a couple of comments regarding naming and docstrings to make things clearer.

aiidalab_widgets_base/utils/exceptions.py

aiidalab_widgets_base/computational_resources.py

danielhollas · 2022-12-07T11:38:34Z

aiidalab_widgets_base/computational_resources.py

+            return False
+        else:
+            return True
+
    def on_setup_computer(self, _=None):


I am a bit confused here. The function name is on_setup_computer, but the doc string says Create a new computer. What is the main purpose of this function?

The main purpose of this function is when the "setup computer" button is clicked, setup (create) a new computer.

The main purpose of this function is when the "setup computer" button is clicked, setup (create) a new computer.

yep, that is correct, but this is also called when running quicksetup. So I can't rename it to something like on_setup_computer_button_clicked

@yakutovicha thanks for the context. Okay, in that case it would be great to expand the doc string on this function to explain when this function is called. This widget is doing so many things that it is beneficial for things to be documented well.

danielhollas · 2022-12-07T11:56:41Z

One of the tests seems to be broken, looks like the structures.ipynb notebook is not working.

unkcpz · 2022-12-07T17:16:53Z

@yakutovicha I make a commit to fix the failed notebook test, hope you don't mind.

aiidalab_widgets_base/computational_resources.py

yakutovicha · 2022-12-08T16:29:08Z

suggestions applied, review re-requested.

danielhollas

@unkcpz @yakutovicha I've looked through the code, and the only callbacks that seem to be registered in _on_setup_computer_success are calls that refresh the widgets' state. But in that case, I don't think these should be run if the computer already exists in the first place. Also, the name on_setup_computer_success implies to me that they should only be run when a new computer is created.

The same comment probably applies to on_setup_code_success callbacks.

Since this PR is about linters, I do not want to block it on this issue, but it should be addressed in a separate PR since we've already dug into it anyway.

danielhollas · 2022-12-08T16:46:47Z

aiidalab_widgets_base/computational_resources.py

-            self.message = f"Unable to store the computer: {err}."
-            return False
-
-        if self._configure_computer(computer):


@yakutovicha it looks like you deleted the call to _configure_computer()

I am also curious about this, I think this is needed. I am pretty sure this PR has some problem, I test it locally and it failed. Hope the test can catch the issue. I'll try to fix those then.

It would be good if you could verify that your new tests fail when this line is deleted. :-)

Great! that exactly manifest the bug. Add the configure function back to solve the issue. You can find the change from last commit 1c568d3

I also change a bit of the setup-configure flow, I combine the computer "setup" and "configure" in the block of a try, which only store the computer node when the setup and the configure are both successful. I did this because we only provide the widget called "Computer Setup" but no "computer configure". When the computer is setup and stored but the configure phase is not finished. The computer in the database has the UUID but leaves it in a very strange state and can not be used for further code setup.

aiidalab_widgets_base/computational_resources.py

unkcpz · 2022-12-09T17:03:32Z

I've looked through the code, and the only callbacks that seem to be registered in _on_setup_computer_success are calls that refresh the widgets' state. But in that case, I don't think these should be run if the computer already exists in the first place

Why you think this should not be run when the computer already exists? The function _run_callbacks_if_computer_exists is called in two condition, if the computer exists, it is run to update the computer dropdown of the code setup widget. If the computer not exists, it is run after the computer is create/setup and then update the computer dropdown widget of the code setup widget. Do you aware that the refresh is not refresh the computer setup widget but the code setup widget, maybe this is why you are confused?

danielhollas · 2022-12-11T02:01:50Z

Why you think this should not be run when the computer already exists? The function _run_callbacks_if_computer_exists is called in two condition, if the computer exists, it is run to update the computer dropdown of the code setup widget.

Well, my thinking was that if the computer already exists, it would have been in the Dropdown already so no need to refresh. But I guess the computer could be create somehow outside of the widget, making the UI not up to date. Anyway, feel free to ignore this.

danielhollas

@unkcpz thanks for finishing this!

unkcpz · 2022-12-11T09:04:23Z

@danielhollas thanks for the review. @yakutovicha I'll let you have a final check and merge the PR.

yakutovicha · 2022-12-12T08:10:06Z

All good, @unkcpz @danielhollas! Thanks a lot for your help!

danielhollas · 2022-12-13T10:38:20Z

aiidalab_widgets_base/computational_resources.py

@@ -835,14 +835,13 @@ def _configure_computer(self, computer: orm.Computer):
            "use_login_shell": self.use_login_shell.value,
            "safe_interval": self.safe_interval.value,
        }
-        if "user" in sshcfg:


@unkcpz I suspect this change broke the setup for local computer, reported in #417.
Could you please revert it and and a test for localhost computers?
(not a huge priority for me)

Ha! You are correct, that makes sense, I'll reconsider how this part of the code should work. It currently leads to two new small issues.

As mentioned in aiidalab/aiidalab-qe#388, the workchain selector is one of the sources lead to the memory leak. It uses a spare threading to update the list which is not fully necessary because the `refresh` button is already served for this purpose. In this PR we did not only remove the threading but also decouple the QeAppWorkChain specific code out as the sub-class, so the WorkChainSelector can be re-used through inherited by other apps.

yakutovicha added 2 commits November 30, 2022 10:21

Applying suggestions by flake8

d977100

yakutovicha marked this pull request as ready for review November 30, 2022 09:58

yakutovicha requested review from unkcpz and danielhollas November 30, 2022 10:04

danielhollas requested changes Nov 30, 2022

View reviewed changes

Update aiidalab_widgets_base/process.py

a18b193

Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>

yakutovicha added 2 commits December 6, 2022 14:50

Add a hook to strip output from Jupyter and IPython notebooks

08044f8

Apply notebook output strip hook.

32af264

Fix an exception type: ValueError->TypeError

87f5f4c

yakutovicha added 2 commits December 6, 2022 15:43

Fix TC003

7e0ff04

Fix TC101 errors

e32cd75

yakutovicha requested a review from danielhollas December 6, 2022 15:01

unkcpz added this to the v2.0.0-final milestone Dec 7, 2022

danielhollas requested changes Dec 7, 2022

View reviewed changes

Fix test, typo of importer type check

5fd173f

danielhollas reviewed Dec 7, 2022

View reviewed changes

aiidalab_widgets_base/computational_resources.py Show resolved Hide resolved

yakutovicha added 2 commits December 8, 2022 17:09

Merge branch 'master' into chore/update-readme-remove-prospector-yaml

017953c

Apply suggestions from the review.

6e2c427

yakutovicha requested a review from danielhollas December 8, 2022 16:28

danielhollas requested changes Dec 8, 2022

View reviewed changes

Merge branch 'master' into chore/update-readme-remove-prospector-yaml

e5d5a7a

danielhollas mentioned this pull request Dec 9, 2022

Fix/406/default calc job plugin #409

Merged

unkcpz added 3 commits December 9, 2022 17:10

r-d

fe3fdf9

Merge branch 'master' into chore/update-readme-remove-prospector-yaml

4025dde

add the configuration back bind it to setup

1c568d3

unkcpz requested a review from danielhollas December 10, 2022 22:56

danielhollas approved these changes Dec 11, 2022

View reviewed changes

yakutovicha merged commit d48415a into master Dec 12, 2022

yakutovicha deleted the chore/update-readme-remove-prospector-yaml branch December 12, 2022 08:14

danielhollas reviewed Dec 13, 2022

View reviewed changes

Work on pre-commit hooks, small readme update. #401

Work on pre-commit hooks, small readme update. #401

Conversation

yakutovicha commented Nov 30, 2022 • edited by danielhollas Loading

yakutovicha commented Nov 30, 2022

danielhollas commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha commented Nov 30, 2022

danielhollas commented Dec 1, 2022 • edited Loading

danielhollas commented Dec 1, 2022

danielhollas commented Dec 1, 2022

yakutovicha commented Dec 6, 2022

yakutovicha commented Dec 6, 2022

yakutovicha commented Dec 6, 2022

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas commented Dec 7, 2022

unkcpz commented Dec 7, 2022 • edited Loading

yakutovicha commented Dec 8, 2022

danielhollas 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

unkcpz commented Dec 9, 2022

danielhollas commented Dec 11, 2022

danielhollas left a comment

Choose a reason for hiding this comment

unkcpz commented Dec 11, 2022

yakutovicha commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha commented Nov 30, 2022 •

edited by danielhollas

Loading

yakutovicha Nov 30, 2022 •

edited

Loading

danielhollas commented Dec 1, 2022 •

edited

Loading

yakutovicha Dec 8, 2022 •

edited

Loading

unkcpz commented Dec 7, 2022 •

edited

Loading