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

conda-forge build of fastmri? #328

Open
2 tasks done
gschramm opened this issue Dec 12, 2023 · 6 comments
Open
2 tasks done

conda-forge build of fastmri? #328

gschramm opened this issue Dec 12, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@gschramm
Copy link

Please:

  • Check for duplicate requests.
  • Describe your goal, and if possible provide a code snippet with a motivating example.

Hi fastmri developers,

what about building fastmri also as conda-forge package?
Since it is pure python and has only minimal dependencies, creating
a conda-forge recipe should be easy - and obviously I am willing to help.

Georg

@gschramm gschramm added the enhancement New feature or request label Dec 12, 2023
@mmuckley
Copy link
Contributor

Hello @gschramm, that sounds great! At this point I am mostly maintaining the repo and not adding features, but I could review your implementation if you'd like to work on it.

@gschramm
Copy link
Author

Sounds great. Before creating the conda-forge "recipe" a few short questions:

  • Which pytorch version required?
  • Which of those tests should be run when building the package?

@gschramm
Copy link
Author

Sounds great. Before creating the conda-forge "recipe" a few short questions:

* Which pytorch version required?

* Which of [those tests](https://github.com/facebookresearch/fastMRI/tree/main/tests) should be run when building the package?

Just found the requirements for install / testing here

@gschramm
Copy link
Author

@mmuckley I did a generate a first conda-forge fastmri recipe using grayskull based on the pypi package.
Unfortunately, it seems that the run requirement runstats is not (yet) on conda-forge.
If this is an essential requirement that cannot be replaced, we have to get it on conda-forge first.

@gschramm
Copy link
Author

@mmuckley conda-forge build is on its way :)
Why do you have different reqs for install and testing in setup.cfg? Right now some tests in test_modules.py fail with newer versions of pytorch-lightning and torch-vision which seems dangerous to me (the install reqs. allows newer versions ...)

Are the tests in tests_modules.py essential or can they be skipped?

@mmuckley
Copy link
Contributor

mmuckley commented Jan 9, 2024

Hello @gschramm, happy new year :).

I apologize for not replying faster - I was on vacation at the end of the year, and the first week of this year has been busy with performance reviews.

The different requirements are because we want a standard test bed to evaluate all aspects of the package, but we don't want to impose those same packages on users. Usually users will want to choose their own package versions.

Since we do distribute the PyTorch Lightning code we probably should get that working or move it into an examples folder. At this point I might prefer to keep it where it is, since it's been there awhile and there might be people building off of it. In that case, we'd have to update that code for the current version of PyTorch Lightning.

If you want, you can open your PR with those tests breaking and we can look at everything else, and maybe I could spend some time at some point to fix PyTorch Lightning (... again.... :( ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants