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

MNT Improve the GitHub Actions #472

Merged
merged 29 commits into from
Sep 3, 2023
Merged

MNT Improve the GitHub Actions #472

merged 29 commits into from
Sep 3, 2023

Conversation

bruAristimunha
Copy link
Collaborator

@bruAristimunha bruAristimunha commented Aug 30, 2023

This is related to #471, #464 and a lot of headaches. We are having a small crisis with version management of the libraries we depend on. It's been an unexpected journey, a little inconvenient and irritating to deal with.

From the beginning, we realized that the workflow was broken at the conda step. We switched to Mamba in PR #461, and Mamba managed to solve it.

After that, we get an extra mandatory skorch parameter without any clear explanation. I tried to find the root of the problem in skorch, but since we didn't notice it right away, git blame doesn't point to the root (related with skorch-dev/skorch#1016). The change was not intentional on the skorch side. I couldn't solve it, and consequently, we now have the classes parameter in our EEGClassifier.

After that, outside of CI, we noticed that some acceptance tests broke. The training curves generated were different from what was expected in the tests, which should not happen. During this investigation process, I realized that this only affects Python versions other than 3.7, but since our CI was only configured on Python 3.7, I don't know how long we've had this error.

After that, I opened this PR to try to improve Git Action and try to resolve everything related.

I will need a beer after this, hahah

Explaining a little the PR and why I am changing a lot of stuffs @agramfort and @robintibor.

@bruAristimunha bruAristimunha marked this pull request as ready for review August 30, 2023 21:15
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #472 (7932e1f) into master (550ce1b) will decrease coverage by 0.90%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   84.41%   83.52%   -0.90%     
==========================================
  Files          60       60              
  Lines        4357     4388      +31     
==========================================
- Hits         3678     3665      -13     
- Misses        679      723      +44     

@bruAristimunha bruAristimunha changed the title Improve the GitHub MNT Improve the GitHub Actions Aug 31, 2023
@bruAristimunha
Copy link
Collaborator Author

Sorry @agramfort, I didn't notice that you had already approved.

@bruAristimunha bruAristimunha merged commit 44a3cb3 into master Sep 3, 2023
14 of 15 checks passed
@bruAristimunha bruAristimunha deleted the improve_the_github branch September 3, 2023 21:29
@bruAristimunha bruAristimunha mentioned this pull request Sep 4, 2023
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.

None yet

2 participants