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

WIP API for Yolact, python package #323

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

breznak
Copy link
Contributor

@breznak breznak commented Feb 7, 2020

WIP
Aim of this PR is to have Yolact used (useful) as a python package, interfacing with other py code with API (ideally as close to pytorch.nn as possible)

  • init() improvements
  • eval()
  • train()
    • might be too complicated
  • get rid of global variables
  • make .cuda() optional, depending on device_type chosen (cpu,gpu,tpu)

Fixes #299
For #300

so it's done automatically, not by hand after init.
yolact.py Outdated
self.init_weights(backbone_path='weights/' + cfg.backbone.path)

# GPU
#TODO try half: net = net.half()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off topic, but has anyone tried running the models as half/fp16? Generally improves performance,mem footprint.

Copy link
Contributor Author

@breznak breznak left a comment

Choose a reason for hiding this comment

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

Please review if you have time.

  • The 1st commit improves constructor.

  • the other commits separate local nn.Module classes to standalone files, and should improve/avoid global cfg requirement.

  • in the following commits I'll focus on bringing eval.py to Yolact.eval()

data/config.py Outdated Show resolved Hide resolved
yolact.py Show resolved Hide resolved
yolact.py Outdated Show resolved Hide resolved
@@ -564,20 +235,21 @@ def freeze_bn(self, enable=False):
def forward(self, x):
""" The input should be of size [batch_size, 3, img_h, img_w] """
_, _, img_h, img_w = x.size()
cfg = self.cfg
cfg._tmp_img_h = img_h
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 sometimes use this "hack" instead of changing the whole block from cfg to self.cfg, is that ok, style-wise?

Copy link
Owner

Choose a reason for hiding this comment

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

Should be okay, since self.x is always so ugly (I really wish Python worked like other OOP languages in that regard).

Copy link
Owner

Choose a reason for hiding this comment

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

Though it looks like you use both cfg and self.cfg in the same function here. I'd pick one and stick with it.

@abhigoku10
Copy link

@breznak is this done cna we use yolact as an api , is it the same for yolact++ also ?? great work

@breznak
Copy link
Contributor Author

breznak commented Feb 9, 2020

can we use yolact as an api , is it the same for yolact++ also ?

yes, this is intended for both yolact, yolact++

Copy link
Owner

@dbolya dbolya left a comment

Choose a reason for hiding this comment

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

I think the direction so far is good, but I'm wondering if it's worth it to have all the modules own the config object vs. just having one global config object. There's so much you'd need to change in order to be able to run two completely distinct configs at the same time, that it's probably just not worth it. And who really needs to have to separate versions of Yolact running at once?

Ok I guess maybe it would actually be useful if you have two different types of images I guess. Hmm, should we expend the effort to support that?

Apart from that, let's also take this oppertunity to reduce the global side effects and to add arbitrary device support (basically change all the .cuda() and fix all the tensor initializations). I'll also try to contribute to this PR when I have time.

yolact.py Show resolved Hide resolved
@@ -396,18 +44,31 @@ class Yolact(nn.Module):
- pred_aspect_ratios: A list of lists of aspect ratios with len(selected_layers) (see PredictionModule)
"""

def __init__(self):
def __init__(self,
config_name="yolact_base_config",
Copy link
Owner

Choose a reason for hiding this comment

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

Ideally, you wouldn't have to modify config.py to use YOLACT as a pip package. Thus, I think this should accept either the name of the config or a config object (i.e., Union[str, Config]). This would require a similar change in set_cfg too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accept either the name of the config or a config object (i.e., Union[str, Config]).

isn't that what's there now? Name of the config. I can change to the (str, Config) but that seems the same to me. I was thinking about ommiting the set_cfg() and expecting the user to import/create the config they want and just pass it here directly. So from data.config import yolact_base; net = Yolact(yolact_base) ?

yolact.py Outdated Show resolved Hide resolved
yolact.py Outdated

# GPU
#TODO try half: net = net.half()
self.cuda()
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably either pass in the device as an argument or let the user handle this externally. Just calling .cuda() has pytorch pick a GPU, and it restricts future CPU or TPU support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and to add arbitrary device support (basically change all the .cuda() and fix all the tensor initializations).

I'll need some guidance on this. Do you mean just add Yolact(device="gpu/cpu/tpu") and handle these accordingly?

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 like this and prepared support for the feature is in 236352d ,
But there's a lot of .cuda() calls in the code so far, even in external DCNv2. I'd prefer to finish this feature in a separate PR later.
Btw, the alternative to .cuda() is .to(device="gpu") ..if so, that's what we'd use.

yolact.py Outdated
# GPU
#TODO try half: net = net.half()
self.cuda()
torch.set_default_tensor_type('torch.cuda.FloatTensor')
Copy link
Owner

Choose a reason for hiding this comment

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

If this didn't break anything, then it's fine, but this should probably go at the start of init not here.

@@ -564,20 +235,21 @@ def freeze_bn(self, enable=False):
def forward(self, x):
""" The input should be of size [batch_size, 3, img_h, img_w] """
_, _, img_h, img_w = x.size()
cfg = self.cfg
cfg._tmp_img_h = img_h
Copy link
Owner

Choose a reason for hiding this comment

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

Should be okay, since self.x is always so ugly (I really wish Python worked like other OOP languages in that regard).

@@ -564,20 +235,21 @@ def freeze_bn(self, enable=False):
def forward(self, x):
""" The input should be of size [batch_size, 3, img_h, img_w] """
_, _, img_h, img_w = x.size()
cfg = self.cfg
cfg._tmp_img_h = img_h
Copy link
Owner

Choose a reason for hiding this comment

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

Though it looks like you use both cfg and self.cfg in the same function here. I'd pick one and stick with it.

data/config.py Outdated Show resolved Hide resolved
print('Multiple GPUs detected! Turning off JIT.')

ScriptModuleWrapper = torch.jit.ScriptModule if use_jit else nn.Module
script_method_wrapper = torch.jit.script_method if use_jit else lambda fn, _rcn=None: fn
Copy link
Owner

Choose a reason for hiding this comment

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

This part should probably go in its own file, since it should only be loaded once and might need to be used in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why that's important, but like this fccd89b ?

layers/modules/fpn.py Outdated Show resolved Hide resolved
to be continued in following commits
currently only "gpu" is supported.
In future, cpu,tpu.
Use `net = Yolact(device_type="gpu")`
`
solving a FIXME which I thought I cannot access the object.
Thanks dboya for explanation: it's an object wrapping the dict, and I
can access it, just my mistake was relying on an IDE, which cannot see
through it.
and imported where needed
initial python package for YOLACT.
run `pip install .`
And use as `from yolact import Yolact`
@breznak breznak requested a review from dbolya May 27, 2020 17:49
@breznak
Copy link
Contributor Author

breznak commented May 27, 2020

@dbolya when you have time, could we please proceed with this PR and #456 ? With those, the code is already quite useful for API-use, while still not yet a proper python package.

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.

Enh: move train and eval scripts to methods, pip package
3 participants