Skip to content

Conversation

@dchansen
Copy link
Contributor

First of three pull requests (in different repositories) to make Julia in Gadgetron fully featured (and working)

@dchansen
Copy link
Contributor Author

@hansenms Mind giving this a look?

@hansenms
Copy link
Member

I don't think I am well equipped to review this. Have not been doing a lot of Julia. But at a high level, there are no tests here for this code, so I would at least call that out. I am also concerned with versioning and what we are pulling into the Gadgetron, that seems like something that should be buttoned up first. And related, the gadgetron --info call on the gadgetron side is now super slow because of the Julia stuff. So it feels like we have some other stuff to sort through first?

@dchansen
Copy link
Contributor Author

Well, you are the only one who can merge it, so there is that.

There are tests, but they are integration tests for Gadgetron itself. I wanted this PR and a follow-up one in GadgetronExamples to be merged before making the Gadgetron PR.
For the streaming parts, I've found integration tests to be much easier to write and more useful than unit tests.

gadgetron --info is slow because Julia is failing. It is essentially trying to precompile the Julia packages every time you run. With a working Julia it takes less than 2s to run. For comparison, Matlab adds 5s.

But all of this raises the question of how this should be maintained.

The options are:

  1. Remove all of it. The .jl repositories and Julia support in Gadgetron. Say it's outside the scope of the project.
  2. Allow Gadgetron.jl to be maintained outside the Gadgetron organization. Gradient Software/I would then be maintaining this package. Gadgetron itself would rely on specific releases of Gadgetron.jl, and any update would involve a PR to Gadgetron.
  3. Review and approve PRs ;)

I should note that the issues here are the exact same as for Matlab and python. Matlab in particular is a bit of an issue, as it has many users (maybe more than Gadgetron itself).

@hansenms
Copy link
Member

You bring up some good points here and I know that this is all a bit annoying, but I want to take the opportunity to see if we can sort out a better flow for this. First of all, I think it makes sense to make this a repo that others can maintain. We can work offline on the permission side, but that means that we need a flow where when this repo updates, some tests will run. The current flow of updating this with no tests in this repo and then pulling it into the Gadgetron is not great. It is bad for Python and Matlab too, so I would suggest we fix it for Julia now and work to fix it for the others too.

At a high level:

I think this repo should have a pipeline that builds a new Docker image with Gadgetron with Julia support, the configs, test cases, etc. should be in this repo and it should run these tests. This repo will depend on a certain version (commit) of the Gadgetron and will build against that. If we could get that flow working, we now have a pattern where support for new languages, but also just new Gadgets and tools can be supported and maintained.

@dchansen
Copy link
Contributor Author

@hansenms The current pull request adds automatic testing of Gadgetron.jl as a github workflow

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