DICOMcloud / DICOMcloud

Azure friendly DICOMweb part 18 .NET server with qido-rs, wado-rs, stow-rs, wado-uri RESTful implementation
http://dicomcloud.azurewebsites.net/
Apache License 2.0
215 stars 101 forks source link

Dotnet Core #104

Open laredoza opened 3 years ago

laredoza commented 3 years ago

Hi,

Is the application using any windows specific functionality/libraries which would block a dotnet core conversion? Is that something that you'd be interested in?

DANIELMWAMBI commented 3 years ago

I think it depends on fo-dicom

Zaid-Safadi commented 3 years ago

fo-dicom has a dotnet-core version so I don't think it is a limitation. Might need to check the codecs supports though.

At one point I evaluated compiling with dotnet-core and seemed to be possible with no main blockers just some effort in the WebAPI project to make it compatible.

@laredoza yes, it will be interesting to see a ported version of DICOMcloud in dotnet-core. I would welcome a PR or even create a new repo for version 2!

laredoza commented 3 years ago

I'll have a look at converting. Just really depends on time, so no promises.

laredoza commented 3 years ago

Dotnet Core Update

laredoza commented 3 years ago

Hi @Zaid-Safadi , with regards to DICOMcloud.Azure I wonder if you could have a look at the following methods.

Some of the overloads have changed and I haven't worked enough with Azure Blobs. I suspect that you'll be able to make the required changes faster than I could.

You will see that the blob container is now using async methods, i.e Blob.UploadFromStream is now Blob.UploadFromStreamAsync(stream);

I have used Wait() and .Result so that I don't have to change the interfaces. I'd like to make as little changes as possible until there is a running version of the application. Refactor and large changes can happen at that point.

Zaid-Safadi commented 3 years ago

sure @laredoza, I created a new branch "issue104-dotnet-core", can you merge your fork/open a PR into it and I'll update the azure methods?

laredoza commented 3 years ago

I've created a pull request as requested @Zaid-Safadi

laredoza commented 3 years ago

@Zaid-Safadi, please go to www.docker.com and create a repository. Please add me as a collaborator ( laredoza ).

Zaid-Safadi commented 3 years ago

done @laredoza

laredoza commented 3 years ago

What is the repository url @Zaid-Safadi ?

Zaid-Safadi commented 3 years ago

this should be it: dicomcloud/dicomcloud @laredoza
I've added laredoza as an admin to the repo, let me know if you can(not) access

Zaid-Safadi commented 3 years ago

Hi @Zaid-Safadi , with regards to DICOMcloud.Azure I wonder if you could have a look at the following methods.

  • AzureContainer.GetLocations
  • AzureLocation.DoDownload
  • DoDownload.DoGetReadStream
  • AzureStorageService.GetContainers

Some of the overloads have changed and I haven't worked enough with Azure Blobs. I suspect that you'll be able to make the required changes faster than I could.

You will see that the blob container is now using async methods, i.e Blob.UploadFromStream is now Blob.UploadFromStreamAsync(stream);

I have used Wait() and .Result so that I don't have to change the interfaces. I'd like to make as little changes as possible until there is a running version of the application. Refactor and large changes can happen at that point.

@laredoza good news, I updated the Azure methods. Bad news, I used the async/await! I see now your point with using Wait/Result so you won't need to do large refactoring. I missed that and went ahead and changed everything to Wait/Async which was a big change.

I didn't get the chance to test the changes yet so if you prefer for now to just merge the updated Azure functions manually into your branch and update them to async/result to make sure things compile and run first, please do so, or you can merge everything and see how it goes :)

laredoza commented 3 years ago

@Zaid-Safadi that's awesome. We were going to need to do that in any case. I'll merge everything.

laredoza commented 3 years ago

this should be it: dicomcloud/dicomcloud @laredoza I've added laredoza as an admin to the repo, let me know if you can(not) access

Thanks, I'll have a look tonight

laredoza commented 3 years ago

@Zaid-Safadi I've pushed a test docker image.

I've added a docker-compose.yml file.

It will currently install/run a SQL express docker image if you run docker-compose up. We can remove this at a later point if you want. Just makes things easier for me.

To upload the latest version of DICOMcloud:

Overriding appsettings.json In the environment section it is possible to override values as follows: ConnectionStrings:pacsDataArchieve: "Server=localhost1a,1433;Initial Catalog=DICOMcloud;Integrated Security=True;User Id=sa;Password=Password123#"

I've tested this on Ubuntu, not sure about Windows. Should be similar.

laredoza commented 3 years ago

@Zaid-Safadi I've merged your Azure changes but I've had to replace CloudConfigurationManager with IConfiguration in AzureStorageService.

Zaid-Safadi commented 3 years ago

Hi @laredoza , apologies I have been distracted from this, how can I help you move this forward?

laredoza commented 3 years ago

Hi @Zaid-Safadi, I've also had several work-related things that took over my life as well. I should have some time again this coming weekend.

With regards to the status of the conversion: The application starts up and communicates with the database. I was about to start testing the controllers. Quite a bit has been done, but there is still a lot of work left

The major issues that need to be looked at are the following:

  1. DICOMcloud.SqlServerDatabase --> We need to find an alternative for this. I ran the scripts manually. I don't think there is a dotnetcore alternative, but to be honest I didn't spend too much time on this.
  2. DicomExtensions needs a lot of work, it's a replacement for DICOMcloudBuilder.
  3. Implement new file uploads, I've commented the old code out on the Studies and WadoUriController. I've spent no time on this.
  4. Update / convert unit tests.
Zaid-Safadi commented 3 years ago

Hey @laredoza,

  1. DICOMcloud.SqlServerDatabase --> We need to find an alternative for this. I ran the scripts manually. I don't think there is a dotnetcore alternative, but to be honest I didn't spend too much time on this.

I wouldn't worry about DICOMcloud.SqlServerDatabase, this is a SQL project to create the database so it can remain the same.

  1. DicomExtensions needs a lot of work, it's a replacement for DICOMcloudBuilder.

Agree, would be nice to use the built-in dependency injection service in .NET Core

  1. Implement new file uploads, I've commented the old code out on the Studies and WadoUriController. I've spent no time on this.

Not sure I am following. Where is the file upload used in the WadoUriController?

Update / convert unit tests.

Yeah, these can wait but I'll try to help with them too

Thanks for the great effort, @laredoza. I am very excited about this!

Zaid-Safadi commented 5 months ago

@laredoza it has been a long time since our latest update.

I've created a new branch aspnet-core for .net core with all code converted in the controllers and the DICOMcloudBuilder. All unit-tests passed successfully -aside from an XML Unicode issue.

Feel free to try it out and provide any feedback here