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

#1747 Upgrade Rust dependencies #1748

Merged
merged 3 commits into from May 10, 2024

Conversation

ahmedyarub
Copy link
Contributor

@ahmedyarub ahmedyarub commented May 8, 2024

Upgrade Rust dependencies

Pull Request Template

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Fixes #1747

Changes

Upgrade all of the Rust dependencies

Testing

Linking it to FSRS and then to Anki

Upgrade Rust dependencies
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

See my comment bellow, but thanks for the update!

Cargo.toml Show resolved Hide resolved
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

Somehow the CI is broken, maybe there is a dependency not updated in the xtask project? @syl20bnr

@ahmedyarub
Copy link
Contributor Author

Somehow the CI is broken, maybe there is a dependency not updated in the xtask project? @syl20bnr

It looks like that was triggered by the migration from .cargo/config to .cargo/cargo.toml.
Please check the fix which worked on my machine.

@syl20bnr
Copy link
Member

syl20bnr commented May 9, 2024

It does not work on the CI. You can put it back into the config file.

@ahmedyarub
Copy link
Contributor Author

ahmedyarub commented May 9, 2024

It does not work on the CI. You can put it back into the config file.

The problem was that I migrated the crate 1.78.0 which throws warnings around using .cargo/config. The solution is renaming it to .cargo/config.toml and upgrading Rust everywhere, including the the Github Actions.

EDIT: from now on I'll run the Workflows on my fork first. Sorry for the inconvenience.
EDIT2: I'm running the actions on my own fork right now. I contact you once all the tests pass.

@syl20bnr
Copy link
Member

syl20bnr commented May 9, 2024

I'm running the actions on my own fork right now. I contact you once all the tests pass.

Thank you.

Does it work if we create a symlink from config to config.toml to keep both ?

@ahmedyarub
Copy link
Contributor Author

I'm running the actions on my own fork right now. I contact you once all the tests pass.

Thank you.

Does it work if we create a symlink from config to config.toml to keep both ?

Yes after much reading that is required only to preserve compatibility between Rust 1.38.0 and 1.78.0, which we neither need and it might throw wrong warnings. The solution that worked was renaming .cargo/config to .cargo/config.toml.
Btw here are my pipelines and I have a couple of subtle problems dunno if you can help with:
1- Suddenly, it started discovering old typos! That I could fix.
2- The file assets/ModuleSerialization.xml is mentioned in _typos.toml but it has been removed a long time ago.
3- tests (windows-2022, 1.78.0, std) fails due to:

INTEL MKL ERROR: The specified module could not be found. mkl_def.1.dll.
Intel MKL FATAL ERROR: Cannot load mkl_def.1.dll.

@syl20bnr
Copy link
Member

I can reproduce the same error on Windows even on 1.75. Looking into it.

The update of tch on windows gives an error:

INTEL MKL ERROR: The specified module could not be found. mkl_vml_avx2.1.dll.
Intel MKL FATAL ERROR: cannot load mkl_vml_avx2.1.dll or mkl_vml_def.1.dll.
@syl20bnr
Copy link
Member

@ahmedyarub I updated your branch, please note that I rewrote the history and forced push to restart from your first commits so you'll need to hard reset your PR branch locally.

The windows errors comes up with tch 0.16, I reverted it to 0.15, is that OK with you ? For the cargo config file, turns out that we can just keep cargo/config.toml and it is OK. Can you try that it works with you ?

@ahmedyarub
Copy link
Contributor Author

@ahmedyarub I updated your branch, please note that I rewrote the history and forced push to restart from your first commits so you'll need to hard reset your PR branch locally.

The windows errors comes up with tch 0.16, I reverted it to 0.15, is that OK with you ? For the cargo config file, turns out that we can just keep cargo/config.toml and it is OK. Can you try that it works with you ?

You know I reached this library by updating Anki which forced me to update FSRS which forced me to update this lib lol. I would be happy to update tch BUT I lost my job yesterday. Feel free to merge it and in a couple of months there will be much more contributions.

Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.61%. Comparing base (5bbc5ea) to head (e49047d).
Report is 1 commits behind head on main.

Files Patch % Lines
...ates/burn-train/src/renderer/tui/metric_numeric.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1748   +/-   ##
=======================================
  Coverage   86.61%   86.61%           
=======================================
  Files         700      700           
  Lines       83427    83423    -4     
=======================================
+ Hits        72257    72258    +1     
+ Misses      11170    11165    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syl20bnr
Copy link
Member

Oh crap, sorry to hear that you just lost your job🙁. I wish you all the best in your job search.

Just to confirm, FSRS does not rely on tch, so we should be good on that front. I'll go ahead and merge this.

Thank you for all your hard work and good luck!

@syl20bnr syl20bnr merged commit 1073752 into tracel-ai:main May 10, 2024
14 checks passed
@ahmedyarub
Copy link
Contributor Author

Oh crap, sorry to hear that you just lost your job🙁. I wish you all the best in your job search.

Just to confirm, FSRS does not rely on tch, so we should be good on that front. I'll go ahead and merge this.

Thank you for all your hard work and good luck!

I'm so thankful for the warm words, they are really helping me pushing through.
Unfortunately I know very little about your project although I have a master degree in AI.
Long story short, I'll be both using and contributing to it once I'm done with LeetCode and that non-sense.
Keep up the great work!
Just a glimpse of my humble publications (I have much more as a freelancer). I have abandoned research a couple of years ago.
Here are also some of my freelance projects on controlling robots with Continuous time autoregressive recurrent neural networks SCTRNN https://fb.watch/r_fr0SIVq1/

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.

Upgrade all dependencies
3 participants