Add "registry checks" section to the website #271

AhmedBasem20 · 2023-09-13T14:13:10Z

Closes #268
Store fetch warnings in the JSON file and display it on the details page.

Edit:

The current PR preview workflow preview.yml gets the data from gh-pages and builds the react app using it, this works fine for most of the cases because the preview purpose is to show the UI so I thought using up-to-date data was not important, but for a scenario like in this PR, where we add new data to the JSON file and use it for the first time on the website, the preview will not display the new feature.

This is solved in the second commit by using the data fetched in this PR.

github-actions · 2023-09-13T14:14:14Z

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-09-17 14:10 UTC

ltalirz · 2023-09-13T20:16:07Z

Thanks a lot @AhmedBasem20 , this is great!

I have some minor requests

Since you are now using the warning style, you can remove the textual > WARNING! prefix (and I don't mean to strip it off, but to never add it in the first place)
Please delete the comment.yml workflow, it is no longer needed

The final question is whether we want to keep the warnings as high up in the detail page as we currently do. Currently those warnings are not targeting users but rather the developers of the package, so perhaps they are not as important and could be moved to a top-level section at the end of the page.
On the other hand, having the warnings high up in the page may make it more likely for plugin developers to fix them.

I would say let's keep them there for the moment, but let's rename the section title to "Registry checks"

After these minor edits, this is good to merge for me

AhmedBasem20 · 2023-09-13T22:00:13Z

Thanks @ltalirz ,

Please delete the comment.yml workflow, it is no longer needed

I think this will still be needed because the PR preview currently doesn't support deploying from forks.
This will be supported in the upcoming release of https://github.com/rossjrw/pr-preview-action , we should keep it until then, right?

ltalirz · 2023-09-14T08:20:19Z

Thanks @ltalirz ,

Please delete the comment.yml workflow, it is no longer needed

I think this will still be needed because the PR preview currently doesn't support deploying from forks. This will be supported in the upcoming release of https://github.com/rossjrw/pr-preview-action , we should keep it until then, right?

You make a very good point there that I had not considered, but I think plugin developers have not actually looked at these warnings (also, for some reason they don't actually show up in the places where they should; e.g. if you look at #238 it does not have this comment; mostly I receive those via email for every new pull request and they have been more annoying than useful).

I think it is ok that for the moment I will still manually have a look at the output of the fetch step in the github action workflow when someone adds a new plugin (let's be sure that the warnings are still printed there).
And let's open an issue to upgrade the pr preview action once the new release is available.

ltalirz

thanks a lot @AhmedBasem20 !

I will update the email now

AhmedBasem20 · 2023-09-14T10:06:22Z

Very nice @ltalirz , I opened the issue at #272

ltalirz · 2023-09-14T10:11:31Z

aiida-registry-app/src/Components/Details.jsx

@@ -105,6 +106,15 @@ function Details({pluginKey}) {
            </p>
            )}

+            {value.warnings && (


Sorry, while writing the email I realized one more small request.

If there are no warnings, can we just show a "sucess"-style message "All checks passed"?

Many plugins pass all checks, so developers would be looking for this section and not finding it

Are there cases where warnings will be empty for a reason other than everything being fine (some error encountered, so the code does not get to the stage of adding warnings)?

If so, we may need to track in the json somewhere whether the fetch stage was completed for the plugin

If there are no warnings, can we just show a "sucess"-style message "All checks passed"?

Done.

Are there cases where warnings will be empty for a reason other than everything being fine (some error encountered, so the code does not get to the stage of adding warnings)?

Yes this is the case for aiida-fenics for example, the plugin link is broken, but even if the warnings is empty it will not display "All checks passed" because this depends on another condition that the detailed infomration must be fetched first. If not, nothing will be displayed.

Thanks @ltalirz I got your point now, I think there is another issue, putting the registry checks on this section will not work for plugins that have empty metadata key. I'll try to fix this

unkcpz · 2023-09-14T11:38:27Z

Thanks, both, I didn't look at the implementation. But the behavior is a bit strange to me. When I go to the detail page of aiida-catmat which don't have check mark on the index page. It has this "All checks passed!" on the detail page. Which plugin has the warning showing, I just cannot see the feature was supposed to add.

ltalirz · 2023-09-14T11:54:31Z

Thanks, both, I didn't look at the implementation. But the behavior is a bit strange to me. When I go to the detail page of aiida-catmat which don't have check mark on the index page. It has this "All checks passed!" on the detail page. Which plugin has the warning showing, I just cannot see the feature was supposed to add.

Yes, ideally the test-install workflow would also add a warning in the checks pointing out that it failed (the equivalent of a missing checkmark)

ltalirz · 2023-09-15T07:01:59Z

Thanks, both, I didn't look at the implementation. But the behavior is a bit strange to me. When I go to the detail page of aiida-catmat which don't have check mark on the index page. It has this "All checks passed!" on the detail page. Which plugin has the warning showing, I just cannot see the feature was supposed to add.

Yes, there seems to be a bug in the current preview.
E.g. aiida-alloy should have warnings

WARNING! development_status key is deprecated. Use PyPI Trove classifiers in the plugin repository instead. [aiida-aenet]
WARNING! Missing classifier 'Framework :: AiiDA' [aiida-alloy]
WARNING! development_status key is deprecated. Use PyPI Trove classifiers in the plugin repository instead. [aiida-alloy]
WARNING! Entry point 'elastic' does not start with prefix 'alloy.' [aiida-alloy]

Store fetch warnings in the JSON file and display it on the website. Delete comment.yml

…views

unkcpz · 2023-09-16T14:27:24Z

aiida_registry/test_install.py

+    plugins_warnings = REPORTER.plugins_warnings
+
+    for name, warnings in plugins_warnings.items():
+        metadata["plugins"][name]["warnings"] = warnings


Hi @AhmedBasem20, the page build is successful but there's a problem here that the warnings are generated in two steps and test install warning will override the previous warnings. You can append the new message is it was there already.

BTW, I can run aiida-registry test-install on my Apple M1 Macbook, and use the new docker image which has arm64 supported.

# install colima brew install colima # Use new docker sock, the python docker API is use DOCKER_HOST to find the docker.sock echo 'export DOCKER_HOST="unix://$HOME/.colima/default/docker.sock"' >> ~/.zprofile # cd into the folder and run aiida-registry test-install --container-image aiidateam/aiida-core-with-services:edge

BTW, I can run aiida-registry test-install on my Apple M1 Macbook, and use the new docker image which has arm64 supported.

This is exactly what I need for this task😅 Thanks for the reminder!

Yes, I know, I also try to reproduce it and find it takes so long to see what happened. It is nearly impossible to debug over action output 😄

the warnings are generated in two steps and test install warning will override the previous warnings.

Yeah I see, also this function is called inside a loop it should only called once in the last step, I also see that the previous warnings coming from fetch_metadata.py cannot be accessed and seen here, I'll try to figure out why.

Also I see that those test-install messages are actually errors not warnings, should we keep them with warnings or separate them? (i.e. plugin["warnings"], plugin["errors"])

The "permission denied" error we saw yesterday was because the UID of my workstation is 1001 while the container has 1000 as default, so the file can not be written to the folder. I added the write permission and it is all good.

Yes, I know, I also try to reproduce it and find it takes so long to see what happened. It is nearly impossible to debug over action output 😄

I have a very naive way on debugging over github actions by breaking the test-install loop after five plugins for example (https://ahmedbasem20.github.io/aiida-registry/aiida-alloy). This is temporarily faster but I'll defiantly try to set the environment properly.

ltalirz

fantastic, this looks great!

I am approving this - I notice one small issue namely that there are no line breaks in the error message; if that's easy to fix please do so, but if not, no problem

ltalirz · 2023-09-17T13:34:55Z

Great, thanks @AhmedBasem20

I am merging this now; I notice one further small potential for improvement, which is to add a warning in the test-install workflow when we skip testing the plugin (e.g. because of mismatch in python version / planning stage / ...).
E.g. for this one https://aiidateam.github.io/aiida-registry/pr-preview/pr-271/aiida-aenet

But let's do that small fix in a follow-up PR

AhmedBasem20 force-pushed the display-warnings branch 2 times, most recently from 67d4d4e to 840681e Compare September 13, 2023 16:51

AhmedBasem20 requested review from ltalirz and unkcpz September 13, 2023 17:28

AhmedBasem20 force-pushed the display-warnings branch from 840681e to 193cb7b Compare September 13, 2023 22:07

AhmedBasem20 force-pushed the display-warnings branch from 193cb7b to cd9235b Compare September 14, 2023 09:57

ltalirz reviewed Sep 14, 2023

View reviewed changes

AhmedBasem20 force-pushed the display-warnings branch from cd9235b to ef5f433 Compare September 14, 2023 10:25

AhmedBasem20 added 2 commits September 15, 2023 14:13

Add fetch warnings section to the website

3b0a8c8

Store fetch warnings in the JSON file and display it on the website. Delete comment.yml

Remove caching on webpage builds & use the updated data in the PR pre…

1f3cfdd

…views

AhmedBasem20 force-pushed the display-warnings branch 4 times, most recently from d1ff265 to f2cbc5c Compare September 16, 2023 11:22

unkcpz requested changes Sep 16, 2023

View reviewed changes

AhmedBasem20 force-pushed the display-warnings branch 2 times, most recently from 573bd49 to 666e603 Compare September 16, 2023 22:37

AhmedBasem20 requested review from ltalirz and unkcpz September 16, 2023 23:17

ltalirz approved these changes Sep 17, 2023

View reviewed changes

ltalirz mentioned this pull request Sep 17, 2023

Refactoring CI workflows #274

Merged

3 tasks

Add test-install errors to the registry checks.

ffa42e3

AhmedBasem20 force-pushed the display-warnings branch from 666e603 to ffa42e3 Compare September 17, 2023 12:37

AhmedBasem20 requested a review from ltalirz September 17, 2023 13:24

ltalirz changed the title ~~Add fetch warnings section to the website~~ Add "registry checks" section to the website Sep 17, 2023

ltalirz merged commit f182c30 into master Sep 17, 2023
9 checks passed

ltalirz deleted the display-warnings branch September 17, 2023 13:37

AhmedBasem20 mentioned this pull request Sep 25, 2023

Separate error prefix message and error message for different formater #285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "registry checks" section to the website #271

Add "registry checks" section to the website #271

AhmedBasem20 commented Sep 13, 2023 •

edited

Loading

github-actions bot commented Sep 13, 2023 •

edited

Loading

ltalirz commented Sep 13, 2023 •

edited by unkcpz

Loading

AhmedBasem20 commented Sep 13, 2023

ltalirz commented Sep 14, 2023 •

edited

Loading

ltalirz left a comment

AhmedBasem20 commented Sep 14, 2023

ltalirz Sep 14, 2023

ltalirz Sep 14, 2023 •

edited

Loading

AhmedBasem20 Sep 14, 2023

AhmedBasem20 Sep 14, 2023 •

edited

Loading

unkcpz commented Sep 14, 2023

ltalirz commented Sep 14, 2023 •

edited

Loading

ltalirz commented Sep 15, 2023 •

edited

Loading

unkcpz Sep 16, 2023

unkcpz Sep 16, 2023 •

edited

Loading

AhmedBasem20 Sep 16, 2023

unkcpz Sep 16, 2023

AhmedBasem20 Sep 16, 2023

AhmedBasem20 Sep 16, 2023 •

edited

Loading

unkcpz Sep 16, 2023

AhmedBasem20 Sep 16, 2023

ltalirz left a comment

ltalirz commented Sep 17, 2023

Add "registry checks" section to the website #271

Add "registry checks" section to the website #271

Conversation

AhmedBasem20 commented Sep 13, 2023 • edited Loading

github-actions bot commented Sep 13, 2023 • edited Loading

ltalirz commented Sep 13, 2023 • edited by unkcpz Loading

AhmedBasem20 commented Sep 13, 2023

ltalirz commented Sep 14, 2023 • edited Loading

ltalirz left a comment

Choose a reason for hiding this comment

AhmedBasem20 commented Sep 14, 2023

Choose a reason for hiding this comment

ltalirz Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhmedBasem20 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz commented Sep 14, 2023

ltalirz commented Sep 14, 2023 • edited Loading

ltalirz commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz Sep 16, 2023 • edited Loading

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

AhmedBasem20 Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz commented Sep 17, 2023

AhmedBasem20 commented Sep 13, 2023 •

edited

Loading

github-actions bot commented Sep 13, 2023 •

edited

Loading

ltalirz commented Sep 13, 2023 •

edited by unkcpz

Loading

ltalirz commented Sep 14, 2023 •

edited

Loading

ltalirz Sep 14, 2023 •

edited

Loading

AhmedBasem20 Sep 14, 2023 •

edited

Loading

ltalirz commented Sep 14, 2023 •

edited

Loading

ltalirz commented Sep 15, 2023 •

edited

Loading

unkcpz Sep 16, 2023 •

edited

Loading

AhmedBasem20 Sep 16, 2023 •

edited

Loading