Skip to content

Reorganize requirements, make albumentations optional #1969

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 22, 2020
Merged

Reorganize requirements, make albumentations optional #1969

merged 7 commits into from
Jan 22, 2020

Conversation

Erotemic
Copy link
Contributor

The albumentations requires an old version of the imgaug library, which is incompatible with a lot of other software stacks (namely the one I'm using) which uses the latest imgaug library.

It looks like albumentations is not tightly coupled with the system and mmdet can work just fine if it isn't installed. Therefore I'd like to propose to make it an optional requirement instead of a forced requirement.

This PR does that and also reorganizes the requirements into a format that should be easier to use. Namely, requirements are broken down by what they are required for (e.g. runtime, tests, build, and options) and they all live in a requirements folder now. The base requirements.txt file uses the -r syntax to refer to each of those files, so pip install -r requirements.txt will install all requirements.

I added an enhanced version of the parse_requirements function being used in the setup.py script, which handles the -r syntax. I also used extras_require to register each type of requirements separately. So by default when you pip install mmdet you will only get the runtime requirements, but if you pip install mmdet[all] you will get everything. Similarly you can pip install mmdet[optional] to only grab runtime and optional requirements.

hellock
hellock reviewed Jan 11, 2020
View reviewed changes
setup.py Outdated
with open(os.path.join(here, filename), 'r') as f:
requires = [line.replace('\n', '') for line in f.readlines()]
return requires
def parse_requirements(fname='requirements.txt', with_version=False):
Copy link
Member

Choose a reason for hiding this comment

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

Better to set the default value of with_version to True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've generally found that its best to not specify a version in the install_requires. This is mostly due to pip's limited ability to resolve dependencies. I've often found that it causes more problems than it solves. However, doing a pip install -r requirements.txt will respect the version requirements.

That being said, only a few requirements actually have version dependencies in the txt files. Also it looked like the old parsing script did include the versions, so perhaps its ok to include them in this instance. I've enabled it to ensure that the existing behavior is preserved and we can always disable it if there are any problems.

requirements/runtime.txt Outdated
@@ -0,0 +1,10 @@
imagecorruptions
Copy link
Member

Choose a reason for hiding this comment

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

Nice work! imagecorruptions may also be made optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@hellock
Copy link
Member

hellock commented Jan 11, 2020

Awesome! The installation guide may also be updated accordingly.

@Erotemic
Copy link
Contributor Author

I modified INSTALL.md and made imagecorruptions optional.

hellock
hellock reviewed Jan 12, 2020
View reviewed changes
docs/INSTALL.md Outdated
@@ -42,8 +42,7 @@ cd mmdetection
d. Install mmdetection (other dependencies will be installed automatically).

```shell
pip install mmcv
python setup.py develop # or "pip install -v -e ."
pip install -v -e .[all]
Copy link
Member

Choose a reason for hiding this comment

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

For most users, minimal installation should be enough. Maybe we can just set the default command to pip install -v -e .?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. Changed.

@Erotemic Erotemic force-pushed the dev/optional_albumentations branch from 06430d5 to 398d44d Compare January 13, 2020 18:32
@Erotemic
Copy link
Contributor Author

I rebased on the latest master.

@Erotemic Erotemic mentioned this pull request Jan 20, 2020
Closed
hellock
hellock reviewed Jan 21, 2020
View reviewed changes
docs/INSTALL.md Outdated
@@ -42,8 +42,7 @@ cd mmdetection
d. Install mmdetection (other dependencies will be installed automatically).

```shell
pip install mmcv
python setup.py develop # or "pip install -v -e ."
pip install -v -e .
Copy link
Member

Choose a reason for hiding this comment

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

I test the installation in a new environment and find that pip install cython is necessary before installing mmdet, otherwise a ModuleNotFoundError error will be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a step where the user runs pip install -r requirements/build.txt. This will ensure that they have all build requirements before installing mmdetection. That should resolve the issue.

@Erotemic Erotemic force-pushed the dev/optional_albumentations branch from 398d44d to 50ba532 Compare January 21, 2020 14:32
@hellock hellock merged commit b5d62ef into open-mmlab:master Jan 22, 2020
mattdawkins added a commit to VIAME/mmdetection that referenced this pull request Mar 13, 2020
@mattdawkins
Merge remote-tracking branch 'origin/viame/master' into viame/master-old
3eaa2a7 
* origin/viame/master: (28 commits)
  Fix FPN upscale
  Extra compiler args
  VIAME-specific build parameters
  Bump version to 1.0.0 (open-mmlab#2029)
  Fix the incompatibility of the latest numpy and pycocotools (open-mmlab#2024)
  format configs with yapf (open-mmlab#2023)
  options for FCNMaskHead during testtime (open-mmlab#2013)
  Enhance AssignResult and SamplingResult (open-mmlab#1995)
  Fix typo activatation -> activation (open-mmlab#2007)
  Reorganize requirements, make albumentations optional (open-mmlab#1969)
  Encapsulate DCN into a ConvModule & Conv_layers (open-mmlab#1894)
  Code for Paper "Bridging the Gap Between Anchor-based and Anchor-free… (open-mmlab#1872)
  Non color images (open-mmlab#1976)
  Fix albu mask format bug (open-mmlab#1818)
  Fix CI by limiting the version of torchvision (open-mmlab#2005)
  Add ability to overwite existing module in Registry (open-mmlab#1982)
  bug for distributed training (open-mmlab#1985)
  Update Libra RetinaNet config with the latest code (open-mmlab#1975)
  Fix issue in refine_bboxes and add doctest (open-mmlab#1962)
  add link to official repo (open-mmlab#1971)
  ...
ioir123ju pushed a commit to ioir123ju/mmdetection that referenced this pull request Mar 30, 2020
@Erotemic @ioir123ju
Reorganize requirements, make albumentations optional (open-mmlab#1969)
7fd9420 
* reorganize requirements, make albumentations optional

* fix flake8 error

* force older version of Pillow until torchvision is fixed

* make imagecorruptions optional and update INSTALL.dm

* update INSTALL.md

* Add note about pillow version

* Add build requirements to install instructions
mike112223 pushed a commit to mike112223/mmdetection that referenced this pull request Aug 25, 2020
@Erotemic @hellock
Reorganize requirements, make albumentations optional (open-mmlab#1969)
7324f27 
* reorganize requirements, make albumentations optional

* fix flake8 error

* force older version of Pillow until torchvision is fixed

* make imagecorruptions optional and update INSTALL.dm

* update INSTALL.md

* Add note about pillow version

* Add build requirements to install instructions
jben-hun pushed a commit to jben-hun/mmdetection that referenced this pull request Jan 10, 2025
@Erotemic @hellock
Reorganize requirements, make albumentations optional (open-mmlab#1969)
de01523 
* reorganize requirements, make albumentations optional

* fix flake8 error

* force older version of Pillow until torchvision is fixed

* make imagecorruptions optional and update INSTALL.dm

* update INSTALL.md

* Add note about pillow version

* Add build requirements to install instructions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellock hellock hellock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Erotemic @hellock