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

Make dscribe an optional dependency #270

Merged
merged 6 commits into from
Jul 25, 2022
Merged

Make dscribe an optional dependency #270

merged 6 commits into from
Jul 25, 2022

Conversation

janosh
Copy link
Member

@janosh janosh commented Jul 25, 2022

Motivation: This in turn makes numba installation optional which caused issues (see #228 (comment)).

@mkhorton
Copy link
Member

Thanks @janosh, added an extra guard statement to ensure localenv.py still mostly works even if the import fails.

@mkhorton
Copy link
Member

Otherwise if you're happy with this, I can merge.

Tagging issue #261 here too.

@janosh
Copy link
Member Author

janosh commented Jul 25, 2022

Otherwise if you're happy with this, I can merge.

Go ahead!

@mkhorton
Copy link
Member

Just double-checking, I think dscribe should still live inside the pyproject.toml (i.e., it was already optional, but we're removing it from the server install)?

@janosh
Copy link
Member Author

janosh commented Jul 25, 2022

I was confused by the fact dscribe was already marked optional. But also confused about why remove it from the server install?

@mkhorton
Copy link
Member

Yes--worth checking! I was premising this discussion on the assumption that you would like to install crystal toolkit with server options, but that dependency solving is currently broken.

Right now, the default install (without any extras) is really just for use in Jupyter.

The install with server extras included allows you to spin up Dash apps, and I imagine this is the installation most people will need if following, e.g., the web app examples.

dscribe was previously a server dependency, because it is required by the local environment component (specified in localenv.py), which one might use to investigate the local environments of a given Structure object, but was not required just for the pure Jupyter use.

@mkhorton
Copy link
Member

The question, perhaps, is if the installation is failing even when not installing the server extras?

@janosh
Copy link
Member Author

janosh commented Jul 25, 2022

The question, perhaps, is if the installation is failing even when not installing the server extras?

I wasn't specifying any extras so... yes, I think.

@mkhorton
Copy link
Member

Ok, I'll have to take a closer look.

My best guess is that this is happening because of a dependency on mp-api (and emmet-core), i.e. via matminer -- I'm not sure when or why matminer was added as a dependency, but the same issue will be present with the example app too. Hm :/

@mkhorton
Copy link
Member

I think we can still merge this in regardless, because loosening the dependency is still a good idea

@mkhorton mkhorton merged commit 2ad33cc into main Jul 25, 2022
@mkhorton
Copy link
Member

So I think matminer should not be a dependency of emmet-core, asking @munrojm to verify, but may make things easier if this is the case.

@janosh janosh deleted the numba-optional-dep branch July 25, 2022 23:28
brookwander pushed a commit to brookwander/crystaltoolkit that referenced this pull request Sep 8, 2022
…ptional-dep

Make `dscribe` an optional dependency
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.

2 participants