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

Update build matrix and job run conditions #38

Merged
merged 72 commits into from May 10, 2023
Merged

Conversation

rmitsch
Copy link
Collaborator

@rmitsch rmitsch commented May 9, 2023

Description

  • Updates build matrix for test workflow to match spaCy's.
  • Update run conditions for test workflow to run on PRs or on pushes to main.

Types of change

Expanding test suite.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@rmitsch rmitsch added the tests Everything related to the test suite label May 9, 2023
@rmitsch rmitsch self-assigned this May 9, 2023
Comment on lines +74 to +76
python -m pip uninstall dotenv
python -m pip uninstall python-dotenv
python -m pip install python-dotenv
Copy link
Collaborator Author

@rmitsch rmitsch May 9, 2023

Choose a reason for hiding this comment

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

This isn't great, but I don't know how to get around this - there seems to be a version of dotenv installed already that doesn't play nice with the dependency that's expected.

@rmitsch rmitsch marked this pull request as ready for review May 9, 2023 21:40
spacy_llm/compat.py Outdated Show resolved Hide resolved
@adrianeboyd
Copy link
Contributor

I think it's probably more important to the basics merged here than deal with the warning, so maybe for now remove -Werror and just keep an eye on the warnings in the pytest summaries, and worry about this later?

@rmitsch
Copy link
Collaborator Author

rmitsch commented May 10, 2023

I think it's probably more important to the basics merged here than deal with the warning, so maybe for now remove -Werror and just keep an eye on the warnings in the pytest summaries, and worry about this later?

Yeah, I'm trying one last thing. If that doesn't work out, I'll drop -Werror.

@rmitsch
Copy link
Collaborator Author

rmitsch commented May 10, 2023

Ready to be reviewed.

Notes:

  • Specifying warning options (e. g. -Werror) in the pytest call seems to overwrite warning options configured somewhere else (e. g. in pyproject.toml).
  • The regex to capture only DepreciationWarnings with pgk_resources in it didn't quite work, so for now we're ignoring all DepreciationWarnings (will create an issue to fix this in a later version).

Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Looks good to me - agreed with prioritizing getting this merged first, and working more on the warning filters later. Would appreciate a final review by Adriane as well 🙏

[update: I'll go ahead and merge this for now so we can continue to build on it]

@rmitsch
Copy link
Collaborator Author

rmitsch commented May 10, 2023

And thanks again to @adrianeboyd for your help with this!

@svlandeg svlandeg mentioned this pull request May 10, 2023
3 tasks
@svlandeg svlandeg merged commit f18d732 into main May 10, 2023
17 checks passed
@svlandeg svlandeg deleted the chore/expand-build-matrix branch May 10, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Everything related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants