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 missing dependencies #57

Merged
merged 3 commits into from
Jan 30, 2025
Merged

Conversation

sarah-tuva
Copy link
Contributor

Hello! Thanks for creating this tool! I ran into two issues when attempting to use your project.

  1. I added pytest to the required packages list since it was missing.
  2. I added a range for numpy because there is a known issue with the version that pandas installed. I received this error when trying to run things: ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

https://stackoverflow.com/questions/78650222/valueerror-numpy-dtype-size-changed-may-indicate-binary-incompatibility-expec

Adding a specific version range for numpy to the required packages list resolved the issue for me.

@LisaWellman
Copy link
Contributor

Thanks for your contribution! I see the build failed with conflicts, would you be able to address?

@dixonwhitmire
Copy link
Member

Thanks for your contribution! I see the build failed with conflicts, would you be able to address?

Yes thank you very much for the PR!

I had a question regarding the pytest addition. We currently support pytest as a "dev" extra. This allows us to include it for local development environments, while excluding it when used "in production" or a deployed environment where tests aren't appropriate.

[options.extras_require]
dev = pytest >=7.1, <8.0;flake8 >=4.0, <5.0;autopep8 >=1.6, <2.0;isort >= 5.10, <6.0
notebook = jupyterlab
optimized-streaming = smart_open >=6.2.0

The quickstart/setup instructions should provide instructions on how to get the local environment setup with "dev" dependencies.

If this didn't work for you, please let us know and we will address it.

If it does work for you then you can:

  • remove the pytest library from "install requires"
  • increment the pytest versions in this options.extras_require section.

Thanks!

Copy link
Member

@dixonwhitmire dixonwhitmire left a comment

Choose a reason for hiding this comment

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

I had a comment regarding the pytest version and we also need to see what's gong on with the tests.

@sarah-tuva
Copy link
Contributor Author

I went ahead and removed pytest from the setup config. I didn't see any instructions on how to install dev tools. Did I miss that somewhere?

I just noticed that it was a command listed in the README Quickstart guide. I was confused for a minute when I tried to run those commands and immediately ran into missing dependency error.

@dixonwhitmire
Copy link
Member

I went ahead and removed pytest from the setup config. I didn't see any instructions on how to install dev tools. Did I miss that somewhere?

I just noticed that it was a command listed in the README Quickstart guide. I was confused for a minute when I tried to run those commands and immediately ran into missing dependency error.

Hi @sarah-tuva thank you for making the changes! Would you be able to make a final change and implement the version in this file - https://github.com/sarah-tuva/CsvToFHIR/blob/main/src/linuxforhealth/csvtofhir/__init__.py to 1.3.0 please?

This will allow us to merge in the changes and have those associated with a new version.

Thanks!

@sarah-tuva
Copy link
Contributor Author

No problem! Version updated in the branch.

@dixonwhitmire dixonwhitmire merged commit 2f38fcb into LinuxForHealth:main Jan 30, 2025
2 of 3 checks passed
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.

3 participants