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

add test into ci #31

Merged
merged 12 commits into from
Aug 29, 2023
Merged

add test into ci #31

merged 12 commits into from
Aug 29, 2023

Conversation

asukaminato0721
Copy link
Collaborator

@asukaminato0721 asukaminato0721 commented Aug 25, 2023

This may has some problems about floating point number. Let's try it.

@asukaminato0721
Copy link
Collaborator Author

asukaminato0721 commented Aug 25, 2023

---- cosine_annealing::test_lr_scheduler stdout ----
thread 'cosine_annealing::test_lr_scheduler' panicked at 'assertion failed: `(left == right)`
  left: `[0.1, 0.09045084971874769, 0.06545084971874847, 0.03454915028125373, 0.009549150281252953, 0.0, 0.00954915028125269, 0.034549150281252376, 0.0654508497187476, 0.09045084971874953, 0.10000000000000367]`,
 right: `[0.1, 0.09045084971874785, 0.06545084971874875, 0.034549150281253875, 0.009549150281252989, 0.0, 0.009549150281252692, 0.03454915028125239, 0.06545084971874746, 0.09045084971874952, 0.10000000000000353]`', src/cosine_annealing.rs:76:5

linux and macos have different floating point implementation. So instead of assert_eq, we need to use something like eps.

@asukaminato0721 asukaminato0721 marked this pull request as ready for review August 25, 2023 06:18
@asukaminato0721
Copy link
Collaborator Author

it seems that linux cache is empty

Run actions/cache@v3
Received 211 of 211 (100.0%), 0.0 MBs/sec
Cache Size: ~0 MB (211 B)
/usr/bin/tar -xf /home/runner/work/_temp/c5ec5a79-37aa-4d5d-93e5-701f2ffcc443/cache.tzst -P -C /home/runner/work/fsrs-optimizer-burn/fsrs-optimizer-burn --use-compress-program unzstd
Cache restored successfully
Cache restored from key: Linux-rust-release-v5-

@Colerar
Copy link

Colerar commented Aug 25, 2023

Currently, the CI configuration seems a bit redundant, we can only run cargo fmt and cargo clippy in Ubuntu, and run test for all targets, this can reduce the CI execution time.

And it may be best to use https://github.com/marketplace/actions/rustup-toolchain-install to ensure all tool versions are aligned and up to date.

@dae
Copy link
Collaborator

dae commented Aug 26, 2023

#33 should fix the caching issue. Agreed that pinning Rust is a good idea, but rust-toolchain.toml is a better choice than a GitHub action, as it also works when people test locally.

@dae
Copy link
Collaborator

dae commented Aug 26, 2023

Regarding tests on multiple platforms, and building in release mode:

  • Having a fast feedback cycle in CI checks is good for developer productivity, so ideally at least one of the platforms will a) compile in debug mode and b) not run any expensive tests.
  • The current tests based on the sample .anki21 file are good for ensuring we don't make regressions as we refactor, but it's not ideal to have to download a file, compile in release mode, and wait for the processing to finish. I'd recommend this test file is either opt-in with an env var so it is only used for local testing, or the test is run in a separate workflow so that there's still a fast fmt+clippy check.

@dae
Copy link
Collaborator

dae commented Aug 27, 2023

I've pushed some changes into #32 that uses the file to run the tests, but skips the training test, so things are still nice and fast when the cache is fresh. If you guys approve of those changes, maybe this PR could be adjusted to focus on separate workflows for running the tests on Mac/Windows, and maybe a separate one for running the training too.

@asukaminato0721
Copy link
Collaborator Author

If you guys approve of those changes, maybe this PR could be adjusted to focus on separate workflows for running the tests on Mac/Windows, and maybe a separate one for running the training too.

I am ok with that :) ,feel free to modify this PR.

@dae
Copy link
Collaborator

dae commented Aug 29, 2023

Linux 34s, Mac 1m, Windows 3:30. 😅

@L-M-Sherlock L-M-Sherlock merged commit e917c0e into open-spaced-repetition:main Aug 29, 2023
3 checks passed
@asukaminato0721 asukaminato0721 deleted the add-test branch August 30, 2023 03:41
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.

4 participants