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 CI for windows runner #1458

Merged
merged 61 commits into from
Oct 29, 2021
Merged

Add CI for windows runner #1458

merged 61 commits into from
Oct 29, 2021

Conversation

lalitpagaria
Copy link
Contributor

@lalitpagaria lalitpagaria commented Sep 14, 2021

Download archive from url without temp file along with adding CI for win & mac
To resolve #1445

Proposed changes:

  • Sometime on windows python process doesn't have permission to create temp file hence extracting archive directly via in memory stream remove the need of temp file.
  • Adding CI for Windows and Mac, but adding it in ci.yml is bit complex so will resolve issue step by step.

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@lalitpagaria
Copy link
Contributor Author

@oryx1729 @julian-risch Please review this PR.
Specially CI part. There are few changes required before making it operational for Win & Mac platform.

requirements.txt Outdated Show resolved Hide resolved
@lalitpagaria lalitpagaria changed the title Feat: Download archive from url without temp file along with adding CI for win & mac WIP: Add CI for windows runner Sep 17, 2021
@lalitpagaria lalitpagaria changed the title WIP: Add CI for windows runner Add CI for windows runner Oct 23, 2021
@lalitpagaria
Copy link
Contributor Author

lalitpagaria commented Oct 23, 2021

@tholor This PR is about to complete. As windows github env does not support Linux containers, we can't start Milvus, Weavite and GraphDB, Tika containers. Please suggest way to disable these test for windows platform (ie what command line argument need to pass?). Also not able to make crawler to work via web-driver.

@julian-risch
Copy link
Member

@tholor This PR is about to complete. As windows github env does not support Linux containers, we can't start Milvus, Weavite and GraphDB, Tika containers. Please suggest way to disable these test for windows platform (ie what command line argument need to pass?). Also not able to make crawler to work via web-driver.

I think a clean way to exclude these tests would be to mark the tests or the individual parameter settings that don't work on windows. The pytest documentation tells me that "It is also possible to mark individual test instances within parametrize" https://docs.pytest.org/en/latest/parametrize.html Thereby, we can exclude them when running pytest.

For example,
@pytest.mark.skipif(condition, ..., *, reason=...): skip the given test function if any of the conditions evaluate to True. Example: skipif(sys.platform == 'win32') skips the test if we are on the win32 platform. See https://docs.pytest.org/en/stable/reference.html#pytest-mark-skipif
(taken from here: https://docs.pytest.org/en/latest/example/markers.html)

A slightly different option would be to mark all milvus tests (and parameter settings) and exclude them via something like pytest -m "not milvus"

@lalitpagaria
Copy link
Contributor Author

Thanks @julian-risch
Yes first approach is clean but it is mainly due to windows GitHub runner doesn't support linux containers due to some licensing issue. Otherwise Milvus absolutely work correctly on my windows laptop. Also adding these annotations on each and every test will cause lot code changes on tests. Hence I would prefer second for this task. Please let me know if that is fine.

Also please let me know your view about creating separate windows-ci.yml instead of keeping it in same ci.yml.

@tholor
Copy link
Member

tholor commented Oct 25, 2021

For the document stores it should be sufficient to pass --document_store_type=faiss,memory,elasticsearch (see https://github.com/deepset-ai/haystack/blob/master/CONTRIBUTING.md#running-tests)

This leaves us with tika and graphdb - for both we already have markers so we should be able to exclude them via not tika and not graphdb. So in the end it should be something along these lines:

python -m pytest test_document_store.py --document_store_type=memory,faiss,elasticsearch -m "not tika and not graphdb"

@tholor
Copy link
Member

tholor commented Oct 25, 2021

Also please let me know your view about creating separate windows-ci.yml instead of keeping it in same ci.yml.

As we are not starting the same docker containers as in the linux CI, I think it would be simpler & cleaner to have it in a separate file. Do you see any downsides for it?

@lalitpagaria
Copy link
Contributor Author

Also please let me know your view about creating separate windows-ci.yml instead of keeping it in same ci.yml.

As we are not starting the same docker containers as in the linux CI, I think it would be simpler & cleaner to have it in a separate file. Do you see any downsides for it?

Pros: It will be more clean
Cons:

  • It will have separate file so if we add some changes to linux-ci then we would need to evaluate same for windows-ci

Also not sure to make tree based CI means first we will run type-check, then only we would like to run CI (linux and windows)

@lalitpagaria
Copy link
Contributor Author

@tholor @julian-risch PR is ready for review. One query regarding when to run windows_ci?

As we are using one runner for each test file, most of the time our check will go to Queue mode. So I would suggest to run windows CI only on push to master instead on every PR. It will help in reduction in carbon footprint :)

@tholor
Copy link
Member

tholor commented Oct 26, 2021

So I would suggest to run windows CI only on push to master instead on every PR. It will help in reduction in carbon footprint :)

Yep, sounds good. Let's reduce carbon footprint a bit and only run for every push to master

@lalitpagaria
Copy link
Contributor Author

@tholor Yes everything is complete in this PR. Please review it and merge if you are fine with the changes.

Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looking good already! Left a few minor comments.

.github/workflows/windows_ci.yml Outdated Show resolved Hide resolved
.github/workflows/windows_ci.yml Outdated Show resolved Hide resolved
.github/workflows/windows_ci.yml Show resolved Hide resolved
@lalitpagaria
Copy link
Contributor Author

@tholor Addressed review comments.
This need you input for now CI is passing due to cache. Not sure how to fix properly.
#1458 (comment)

Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

Looking good to me. Thanks a lot @lalitpagaria ! Will hopefully help us to spot more bugs for our windows friends

@tholor tholor merged commit e5b4b62 into master Oct 29, 2021
@tholor tholor deleted the fetch_archive_directly branch October 29, 2021 08:22
@github-actions github-actions bot restored the fetch_archive_directly branch October 29, 2021 08:25
@masci masci deleted the fetch_archive_directly branch August 18, 2022 16:38
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.

Need Help Running Basic QA Notebook on Windows 10
3 participants