Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial pull request focused on integrating pre-commit and addressing several minor issues. #190

Merged
merged 20 commits into from
May 13, 2024

Conversation

mo-dkrz
Copy link
Contributor

@mo-dkrz mo-dkrz commented Mar 28, 2024

This PR introduces pre-commit integration and fixes a number of minor issues. Detailed updates are documented in the CHANGELOG.md.

  • By fixing some minor issues, ci_job is green now

Copy link
Member

@antarcticrainforest antarcticrainforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that all previously defined tests are running, and that we don't overuse types: ignore declaration too much.

@mo-dkrz
Copy link
Contributor Author

mo-dkrz commented Apr 8, 2024

Dear @antarcticrainforest,
I've completed my current work on this repository, addressing the issues you previously mentioned in the pull request. Additionally, I've introduced some new tests to enhance our coverage. Looking ahead, I plan to achieve full coverage after completing my learning and review of the freva-deployment. by the way I added an if condition for docs to not be running on none main branches, because of this, the doc service is blocked.
I would greatly appreciate your review of the changes. If everything is in order, could you please merge the request?

Copy link
Member

@antarcticrainforest antarcticrainforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests need some updates, especially the one that tests if a broken plugin marked itself as broken.

Could you also describe how to set up the pre-commit hooks in the README?

…e and remove if condition in docs service of ci_jobs
@mo-dkrz
Copy link
Contributor Author

mo-dkrz commented May 7, 2024

Dear @antarcticrainforest,

I've found the issue affecting failure (freezed) of docs job in pipeline, which mirrors the same problem we encountered in a unit test where no status other than scheduled could be get when batchmode=True was enabled.

Identified Issues:

  1. The wait function in utils.py lacks a timeout parameter, causing it to hang indefinitely without raising an error. here is the line that keep us waited forever ...

  2. Another related issue involves a 60-second wait which, upon passing the conditional in the wait function, leads to a Plugin did not finish error as detailed here.

Solution:

A straightforward fix to get the documentation building process working is to simply remove these two problematic wait lines. This change has proven to successfully generate the docs in my tests.

However, for a more fundamental solution, we need to investigate why the Plugin status remains stuck at scheduled when batchmode=True. This might involve reviewing how batchmode is handled or making deeper changes to our plugin management logic. For instance:

res = freva.run_plugin("dummypluginfolders", batchmode=True)

If you agree I will comment wait in these two mentioned docstrings to pipeline works for now to be able to merge, then to investigate deeper, we can make an issue to tackle the problem.

@antarcticrainforest
Copy link
Member

That is interesting. I do have a suspicion. I'll check tomorrow morning the docs config. Specifically what sheduler is used when the batchmode=True option gets involved.

@mo-dkrz
Copy link
Contributor Author

mo-dkrz commented May 13, 2024

thanks @antarcticrainforest after seven months finally we got the green pipeline in all services :). I think one of your review requests changrs is just stuck there and has blocked the Merging #190 (review)

@mo-dkrz
Copy link
Contributor Author

mo-dkrz commented May 13, 2024

@antarcticrainforest btw one docs action to main branch is still running: https://github.com/FREVA-CLINT/freva/actions/runs/9061866502/job/24894525377

@antarcticrainforest antarcticrainforest self-requested a review May 13, 2024 13:49
Copy link
Member

@antarcticrainforest antarcticrainforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antarcticrainforest
Copy link
Member

@antarcticrainforest btw one docs action to main branch is still running: https://github.com/FREVA-CLINT/freva/actions/runs/9061866502/job/24894525377

Cancelled.

@mo-dkrz mo-dkrz merged commit a76fbb5 into FREVA-CLINT:main May 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants