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

Panoptic segmentation #96

Merged
merged 96 commits into from Oct 11, 2021
Merged

Panoptic segmentation #96

merged 96 commits into from Oct 11, 2021

Conversation

vniclas
Copy link
Collaborator

@vniclas vniclas commented Jun 23, 2021

Hi,
this PR adds the EfficientPS network. The original repo can be found here.

Please also check issue #90.

Todos:

Known issues:

  • reason for failing tests: 3rd party dependencies assume an existing pytorch installation since they attempt to load torch in their setup.py

@Pavlos-Tosidis
Copy link
Collaborator

Hi,
this PR adds the EfficientPS network. The original repo can be found here.

Todos:

* [ ]  Upload pre-trained models to OpenDR server and adjust the URLs in [efficient_ps_learner.py](https://github.com/tasostefas/opendr_internal/blob/panoptic-segmentation/src/opendr/perception/panoptic_segmentation/efficient_ps/efficient_ps_learner.py). _Where are the instructions to upload something to the server?_

You should upload the files to OpenDR's owncloud, under the FTP server material, with the desired structure. When you upload the files, send me an email and I will take care of uploading the files to the FTP server

@vniclas vniclas force-pushed the panoptic-segmentation branch 2 times, most recently from 87a7bc4 to ecc59d9 Compare June 23, 2021 13:11
@vniclas vniclas changed the title [WIP] Panoptic segmentation Panoptic segmentation Jun 23, 2021
@passalis
Copy link
Collaborator

@vniclas testing is now available again. So, when you are ready, you can use the "test sources" label to run the style checks and "test tools" to run the tool tests.
Please let us know when this PR is ready for review.

@stefaniapedrazzi
Copy link
Collaborator

Is there a particular reason why the mmcv library is used for the example (at least the one is supposed to be added in D7.1) instead of the opencv library that is already a dependency of the opendr toolkit?
We should keep the number of dependencies as low as possible and reuse the same as much as possible.

@vniclas
Copy link
Collaborator Author

vniclas commented Jun 28, 2021

Is there a particular reason why the mmcv library is used for the example (at least the one is supposed to be added in D7.1) instead of the opencv library that is already a dependency of the opendr toolkit?
We should keep the number of dependencies as low as possible and reuse the same as much as possible.

The examples and test files have been changed to cv2 for opening image files. However, we cannot completely remove mmcv as a dependency since the underlying base code relies on the library.

@vniclas vniclas added test sources Run style checks test tools Test the toolkit methods labels Jun 28, 2021
@stefaniapedrazzi
Copy link
Collaborator

I tried to run the sample example but I'm getting some errors:

>>> from opendr.perception.panoptic_segmentation.datasets import CityscapesDataset
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/stefi/develop/opendr_internal/src/opendr/perception/panoptic_segmentation/datasets/__init__.py", line 2, in <module>
    from opendr.perception.panoptic_segmentation.datasets.cityscapes import CityscapesDataset
  File "/home/stefi/develop/opendr_internal/src/opendr/perception/panoptic_segmentation/datasets/cityscapes.py", line 28, in <module>
    from mmdet.datasets import CityscapesDataset as MmdetCityscapesDataset
ModuleNotFoundError: No module named 'mmdet'

It seems that some dependencies are not correctly listed.

@vniclas
Copy link
Collaborator Author

vniclas commented Jun 28, 2021

As written in the known issues section of the PR, the dependencies have not been completely sorted out yet since the base repositories assume a manual installation of pytorch before a successful installation. Just using pip install -r requirements.txt does not work.
If you have any idea how to integrate a 2 step installation process in the OpenDR environment, please let me know.

@stefaniapedrazzi
Copy link
Collaborator

Ok, so the example added in D7.1 it currently doesn't work and cannot be tested.

For installation you can simply add a bash script that will be then called by the main installation system.
However, the OpenDR toolkit should use a single version of pytorch for the whole library.
So the installed version (manually or whatever) should work with all the provided tools and all the methods requirements should be adjusted to use the exact same version.

But what does exactly requires the third party dependency?
A specific version, specific installation setup, specific installation path?

@stefaniapedrazzi
Copy link
Collaborator

stefaniapedrazzi commented Jun 28, 2021

3rd party dependencies require pytorch to be installed first since they attempt to load torch in their setup.py

First with respect of what?
It is probably possible to find a way to specify the installation order even using the requirements.txt system.

@vniclas
Copy link
Collaborator Author

vniclas commented Jun 30, 2021

I checked all 3rd party tools used in this PR. The minimum common requirement for pytorch is torch>=1.4.0, however the pre-trained models that have been made available work only with torch==1.7.x. I have not found any minimum version number specified by the OpenDR toolkit. Could you please point me to some documentation (if it exists)? Are there more restrictions induced by other python libraries or even the base interpreter?

Basically, the problem is that both inplace_abn and mmdet attempt to load torch in their respective setup.py files for building the libraries, i.e., the installation will fail if pytorch has not been installed beforehand.

@stefaniapedrazzi
Copy link
Collaborator

I checked all 3rd party tools used in this PR. The minimum common requirement for pytorch is torch>=1.4.0, however the pre-trained models that have been made available work only with torch==1.7.x. I have not found any minimum version number specified by the OpenDR toolkit. Could you please point me to some documentation (if it exists)? Are there more restrictions induced by other python libraries or even the base interpreter?

The documentation where a first set of dependencies is specified is D2.1:
https://cicloud.csd.auth.gr/owncloud/apps/files/?dir=/OpenDR/Documents/WP2/Deliverables&fileid=48
Here we specified that we would use PyTorch 1.5.0.
Note that if needed, the pytorch version can be changed but it should be changed in all the tools and all the tools needs to support it.

Basically, the problem is that both inplace_abn and mmdet attempt to load torch in their respective setup.py files for building the libraries, i.e., the installation will fail if pytorch has not been installed beforehand.

I would say that the issues is rather in these libraries than in the pip installation system.
But in any case, pytorch is a main requirements of the OpenDR toolkit and can be installed in any case just before the specific tools dependencies, for example by adding it here:
https://github.com/tasostefas/opendr_internal/blob/94f6bc0d9d5f2ca5e68d79394754363ec17600f9/dependencies/install.sh#L13
Would it work in this case?

@vniclas vniclas force-pushed the panoptic-segmentation branch 3 times, most recently from 897a1e8 to 54e14ea Compare June 30, 2021 16:54
@vniclas
Copy link
Collaborator Author

vniclas commented Jun 30, 2021

Thanks, @stefaniapedrazzi! That helped a lot. Sorry that I was not aware of this specification since it is not mentioned in the repo's wiki and I just joined this project one month ago. I added torch==1.5.1 to the install.sh script.

Do you have any idea why clang fails on ubuntu-20, whereas it passes on ubuntu-18? I formatted the files with clang-format-9 on an ubuntu-20 machine.

@stefaniapedrazzi
Copy link
Collaborator

I added torch==1.5.1 to the install.sh script

I just checked the code and all the already merged methods are using torch 1.7.1.
So if possible your script should also install 1.7.1. Otherwise we will have to find a version working for all the tools or adjust some of them.

About the error with clang, it may be that a different version is installed depending on the system.
This can be checked and fixed.
But it seems that your are including lot of files.
Did you copy the files in the algorithm folder from a third party or did you write all of them specifically from the OpenDR toolkit?
If they are copied from another source then they should not follow the OpenDR style and the clang-format test could be skipped for it.
Even better, if they are stored on another GitHub repo, we should linkn to them as a sumodule instead of include the files directly in this repo.

@@ -189,7 +189,7 @@ def infer(self, img):
raise UserWarning("No model is loaded, cannot run inference. Load a model first using load().")
self.model.eval()
predict = self.model(image).argmax(dim=1).squeeze().cpu()
heatmap = Heatmap(predict)
heatmap = Heatmap(predict.numpy())
Copy link
Collaborator Author

@vniclas vniclas Oct 5, 2021

Choose a reason for hiding this comment

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

Note: Fixing this in another package. The PR of bisenet did not implement type checking for the Heatmap class although only Numpy arrays were expected according to the original comments in the code. Thus, the error was not detected in the original PR.

@vniclas vniclas marked this pull request as ready for review October 6, 2021 15:52
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

Looks good overall, thank you! Please double check the default parameters, as I did it very quickly.

docs/reference/efficient_ps.md Outdated Show resolved Hide resolved
docs/reference/efficient_ps.md Outdated Show resolved Hide resolved
docs/reference/efficient_ps.md Outdated Show resolved Hide resolved
docs/reference/efficient_ps.md Outdated Show resolved Hide resolved
docs/reference/efficient_ps.md Outdated Show resolved Hide resolved
tests/test_pep8.py Show resolved Hide resolved
@vniclas
Copy link
Collaborator Author

vniclas commented Oct 8, 2021

Looks good overall, thank you! Please double check the default parameters, as I did it very quickly.

Thanks for your detailed review! I'll fix everything in the course of today.

Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

Thank you!

@vniclas vniclas merged commit f612cbd into master Oct 11, 2021
@vniclas vniclas deleted the panoptic-segmentation branch October 11, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants