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

Do not use cupy-cuda v8, as it contains breaking changes #5194

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

tommilligan
Copy link
Contributor

Description

Do not use cupy-cuda v8, as it contains breaking changes. See #5193 for a full description.

Types of change

Bug fix in the install process.

Checklist

  • I have submitted the spaCy Contributor Agreement.
  • I ran the tests, and all new and existing tests passed. (not done, as no code changes)
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

Closes #5193

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.

Thanks for the report and the fix! Actually, I think we may just fix this in thinc so we don't have to restrict the dependencies here.

@ines
Copy link
Member

ines commented Mar 24, 2020

@svlandeg I guess spaCy and Thinc should match, though? Not sure what takes precedence if we do spacy[cuda90] and spaCy and Thinc define different version ranges here.

Ultimately, it should probably all refer to Thinc and doing spacy[cuda90] should just pull in thinc[cuda90]. But not sure if that works or if it has unintended side-effects?

@ines ines added install Installation issues third-party Third-party packages and services 🔮 thinc spaCy's machine learning library Thinc labels Mar 24, 2020
@svlandeg
Copy link
Member

svlandeg commented Mar 24, 2020

@svlandeg I guess spaCy and Thinc should match, though?

I'm not suggesting to limit the range in Thinc, I'm suggesting / hoping that the fix in explosion/thinc#330 means we won't have to restrict the range at all. Sorry, should have been more clear!

Also, that only works if there are no other breaking changes from cupy-cuda 8.0.0 onwards, ofcourse. [EDIT: but from their release notes, it doesn't look like it]

@tommilligan
Copy link
Contributor Author

Agree that it doesn't look like v8 will have other breaking changes in it. Maybe it would still be good to add <9.0.0 though, so this isn't repeated next time?

@svlandeg
Copy link
Member

Sure, we could do that

@tommilligan
Copy link
Contributor Author

Updated to restrict to <9.0.0. Please merge or close as applicable 🙂

contributions.

* [ ] I am signing on behalf of my employer or a legal entity and I have the
actual authority to contractually bind that entity.
Copy link
Member

Choose a reason for hiding this comment

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

Could you fill in either of these two options by making it [x]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@svlandeg
Copy link
Member

(FYI - I don't think it's useful to force push your changes to "hide" the old commits. If anything, that makes the discussion on the PR & the reviews kind of difficult to follow ;-))

@honnibal honnibal merged commit e904958 into explosion:master Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install Installation issues 🔮 thinc spaCy's machine learning library Thinc third-party Third-party packages and services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spacy[cuda100] installs cupy-cuda100 8.0.0b1, which has a breaking change
4 participants