-
Notifications
You must be signed in to change notification settings - Fork 0
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
Eko 0.12 compatibility #73
Conversation
I would say just try to run, and you will find broken things. Hopefully not that many. In particular, you can learn how to use the new methods from the EKO unit tests. In particular, an EKO is now opened with |
Ok so as far as I understood I don't need anymore the legacy part: |
Not really, from that point of view nothing should have changed... |
I temporarily moved the EKO dependency from the usual semver one to Git, such that unit tests are meaningful, please remember to restore the usual before merging. |
Here there is a good point where to start from: |
Nono I understand. But I don't want to realease this, just merge to main |
In principle that might be fine, however this means that we will not be able to make any other release. TL;DR: we need this branch, but we need more an EKO tested release -> this sets the priorities |
Ok I agree. Then, since I cannot compute stuff as long as eko is not tested, I will help in testing it :) |
Ok now that the tests are succeding and the dependecy to eko has been restored, can we merge this? |
Ok nevermind, it seems that in eko something else is changed and the tests are failing again... |
The problem is clearly with |
I would revert 72f6cf9: notice that tests were actually passing before, so they have been broken by it. The problem was only in the benchmarks, and just concerning |
No, note that the tests are still passing, the problem is the usual with codecov |
Ok, then I change my statement: that commit simply didn't change the status. Benchmarks are still failing because of |
I know, I fixed it now :) (without making other commits, I am a wizard ;) ) |
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
There is one comment from @giacomomagni still open https://github.com/NNPDF/pineko/pull/73/files#r1073591854. After that, I believe we can just merge. |
For that I don't know the answer. You guys know? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only drawback of this PR is that it is bigger than I would have liked. But I'm fine with merging and keep going.
@felixhekhorn @giacomomagni Do you agree that we can merge? |
Here we should fix what is needed to make pineko work with the new eko implementation (in particular with the develop version). @felixhekhorn @alecandido