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

ci/Add Windows pipeline #925

Merged
merged 29 commits into from
Dec 14, 2023
Merged

ci/Add Windows pipeline #925

merged 29 commits into from
Dec 14, 2023

Conversation

Luni-4
Copy link
Collaborator

@Luni-4 Luni-4 commented Nov 2, 2023

Pull Request Template

Checklist

  • Confirm that run-checks script has been executed.

Related Issues/PRs

Provide links to relevant issues and dependent PRs.

Changes

This PR adds Windows CI to burn

Testing

Describe how these changes have been tested.

@Luni-4 Luni-4 marked this pull request as draft November 2, 2023 16:06
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 2, 2023

@nathanielsimard

The Windows CI error is related to a part of burn's code I do not know. It seems there is a missing files problem https://github.com/burn-rs/burn/actions/runs/6735375454/job/18308404823#step:9:2067

If I'm not wrong we can fix the problem using https://doc.rust-lang.org/std/env/fn.temp_dir.html

@nathanielsimard
Copy link
Member

@nathanielsimard

The Windows CI error is related to a part of burn's code I do not know. It seems there is a missing files problem https://github.com/burn-rs/burn/actions/runs/6735375454/job/18308404823#step:9:2067

If I'm not wrong we can fix the problem using https://doc.rust-lang.org/std/env/fn.temp_dir.html

Yeah, we should probably update the tests before merging this PR, using temp_dir is better than actually targeting the directory /tmp which may not exist on Windows.

@Luni-4 Luni-4 force-pushed the windows branch 2 times, most recently from 6314992 to 080d714 Compare November 10, 2023 08:45
@antimora
Copy link
Collaborator

Saw the hack for windows. Please add the context and when this hack should be removed. Preferably we should have an issue or tondo fix the hack. The commit messages are invisible after a merge so we can't rely on them.

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 10, 2023

Saw the hack for windows. Please add the context and when this hack should be removed. Preferably we should have an issue or tondo fix the hack. The commit messages are invisible after a merge so we can't rely on them.

Thanks @antimora for your review! Yep, I will open an issue as soon as the CI for Windows will be finished. For now I'm just investigating which issues will be arising from the execution of these CI runs

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 13, 2023

I cannot detect where the problem is in the burn-wgpu crate, as you can see from the commits histroy I have tried different approaches, but I always obtain the same error: No adapter found for graphics API AutoGraphicsApi

My attempts:

  1. Using the same CI for Windows implemented in wgpu project, but it failed
  2. Update wgpu version to 0.18
  3. Tried to set up only the WGPU_BACKEND: dx12 environment variable

Perhaps the main problem could be in the burn code, or in the wgpu-hal crate which comunicates directly with the device. I do not know. I do not have a Windows machine ready and usable to test burn code, so I can only use this CI machine. If someone is more expert than me with Windows and is willing to help me, I'm saying thanks in advance!

@Luni-4 Luni-4 mentioned this pull request Nov 22, 2023
2 tasks
@syl20bnr
Copy link
Member

Going to look at this one.

@syl20bnr syl20bnr force-pushed the windows branch 20 times, most recently from 3d87367 to 8c1b41d Compare November 24, 2023 16:52
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Dec 14, 2023

@nathanielsimard
Approved for me

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.

LGTM 🙏

@nathanielsimard nathanielsimard merged commit 0b2a938 into tracel-ai:main Dec 14, 2023
14 checks passed
@Luni-4 Luni-4 deleted the windows branch December 14, 2023 23:15
nathanielsimard added a commit that referenced this pull request Dec 14, 2023
nathanielsimard pushed a commit that referenced this pull request Dec 14, 2023
Co-Authored-By: Sylvain Benner <sylvain.benner@gmail.com>
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

4 participants