Skip to content

Conversation

@sarahmcmorgan
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.

@sarahmcmorgan
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!

@sarahmcmorgan
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