search

deepfakes / faceswap

Deepfakes Software For All
https://www.faceswap.dev
GNU General Public License v3.0
52.54k stars 13.24k forks source link

[Discussion] Merging PRs #264

Closed Clorr closed 6 years ago

Clorr commented 6 years ago

Hi guys,

We have quite a lot of PRs by now and we should merge a couple of them. I need your feedback on what is to be merged now and what can wait. Here is my proposal:

To merge right now

256 Add multi-GPU support. => I see no problem

251 Add Improved AutoEncoder model. => I see no problem

258 Histogram matching with option -mh => I see no problem

242 Align Eyes Horizontally After Umeyama => I see no problem

262 Use k-nn for face recognition => not sure if it behaves well, but face filter is not a critical feature and the PR is well made

259 Improving performance of extraction. Two main changes to improve the … => seems fine

217 Update GAN64 to v2 => mature enough now I think, if some bugs left, they will be handled by new PRs

257 Gan v2.1 => redundant with #217 ?

Still under consideration

253 Add image rotation for detecting more faces and dealing with awkward angles => seems fine, maybe need some more feedback

260 Convert: Exclude deleted faces => Maybe needs some more discussion on the workflow (separate tool or option)

255 image sort tool => same as for previous

227 New control for training.sh loop => Those who want this should go there and let us know. If it is ok I'm ok to merge

To be closed

218 Training Data section add a download link convenient download for mai… => specific to chinese users. If someone wants to make a chinese README I would prefer

222 Add free automated flake8 testing of pull requests => If anyone think it brings advantages, let us know

241 MultiGPU plugin => Was here for convenience, thanks @kellurian

248 [Experiment] Retraining decoder to 256x256 => For demo

195 color match A and B set images + performance improvement => Has pros and cons, especially the full load of all images which a showstopper I think

194 measure and output min,max,avg, last train timing => not really needed IMHO, if you want this let us know

151 [Draft] Adding GAN128 => to be closed with the merge of @oatssss

54 [DRAFT] Video conversion #10 => for demo

Let me know if you are ok. Please avoid discussing other's opinion...

ruah1984 commented 6 years ago

GAN 2.1 i think we shall test more to confirm

gdunstone commented 6 years ago

This looks fine to me.

On 9 Mar. 2018 9:48 pm, "ruah1984" notifications@github.com wrote:

GAN 2.1 i think we shall test more to confirm

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/deepfakes/faceswap/issues/264#issuecomment-371779349, or mute the thread https://github.com/notifications/unsubscribe-auth/AEDEePQD8bRP2tDYtpu1Yc3XlXaY0Z6Xks5tcl3zgaJpZM4SkDXp .

torzdf commented 6 years ago

Obviously, I'm biased, but I'd really like to see #253 added, so I can work on other areas. It's an optional flag, and I haven't had any issues with it. In the unlikely event that something does come up, I will be on hand to hotfix.

I should add, you're doing a great job, and it is probably a bit of a headache administrating all of this!

Cheers!

babilio commented 6 years ago

@Clorr what do you think of having a dev/test branch, in that case we could get PR requests in that branch quicker and maybe with less scrutiny, let devs test each other features and integrate them. Then, every so often just clone a good checkpoint to master? This could have less risk on what end users get.

kcimit commented 6 years ago

GAN64 v2, I guess it comes with 128x128 procesing?

iperov commented 6 years ago

255 image sort tool - I wrote it as separated tool as you request @Clorr

Clorr commented 6 years ago

@babilio why not, but I'm not very familiar with that type of workflow, and I thought PRs are already handy as they can be checkout and locally tested by a pull

@kcimit yes, the PR comes with all main GAN versions AFAIK

@iperov I asked you to do a separate tool as you proposed a new command and I don't want to have new commands for too specific behaviors. However if sort can be integrated in the extract, it is even better because it will be integrated seamlessly in the process. But it is up to you to see and discuss with others if you all agree to a common workflow. Note that if there is no consensus about that, we still can just merge the separate tool

Clorr commented 6 years ago

@iperov @bryanlyon @babilio no problem to integrate tool as it is, if someone disagree, plz discuss in the PR

Clorr commented 6 years ago

@i5L4NDOF5T4BiLiTY @kcimit plz don't discuss about workflow here, do it on the PR or in a new issue

Clorr commented 6 years ago

Same for @bryanlyon

kcimit commented 6 years ago

@Clorr , I think this is what we are trying to achieve here, PR from @iperov could be considered as a good start, needs massaging though. But no need to demotivate contribuitors here - some people voiced for it.

Clorr commented 6 years ago

@bryanlyon Ok I understand but it was confusing.

If you are against the PR because the tool has a bug, I'm ok with that and you should tell it in the PR. As long as there are important bugs, the PR won't be merged.

If you are against the tool in general, I can't agree as it is here for convenience. This can help people and I'm fine with that. But I now understand that it was not your point

Clorr commented 6 years ago

@kcimit that's why it is in the under consideration section. It is open, and waits for feedback and improvments

bryanlyon commented 6 years ago

I agree that direct discussion should exist on the individual PRs. This separate discussion is confusing, especially if we want to avoid discussing specific improvements for the individual PRs.

I believe that instead of an issue for merging a collection of PRs, which is obviously causing confusion, we should discuss them on the individual PRs and/or merge into a dev/test branch and then promote to master when appropriate as @babilio suggested.

Clorr commented 6 years ago

I would agree if we were in a company where everyone is similarly involved in the project, but it is a bit different here. This issue is exceptional as we had some old PR not reviewed and quite a lot of new PRs, plus the GAN that is long awaited.

In such an open source project, some PRs are popular and we know quite fast if it is ready or not, for the others, it is not really clear. And a test branch isn't really helpful in these cases because for some PRs, the decision is not to know if they are ready to be released or not, but to know if they have to be present in the project or not, and for that I need to take decisions.

The purpose of this issue is to make clear what actions I intend to take and I'm just waiting for feedback from people who disagree with my decisions.

And the 3 possibilities I have, for each PR is: merge, let it open, close. So I divided this into 3 categories: For To merge right now, they will be merged unless someone convince me that it is not ready. For Still under consideration, they will remain open and under discussion, until they are ready to merge (or are closed because no consensus is made), like any normal PR. So these one continue to live, and no specific action will be taken from my part. For To be closed, they will be closed unless someones tells me it should be retained or merged.

Sorry if I was not clear enough, I understand this can be confusing.

bryanlyon commented 6 years ago

I say we merge all the ones you've suggested as merge, as well as #260 and #252. Though 252 should probably be in a "tools" folder.

One note, if we merge both #256 and #251 we'll need to patch the IAE model for Mutli GPU. I've already done it in the https://github.com/bryanlyon/faceswap/tree/iae_multigpu but I am reluctant to submit a PR for something that relies on two existing PRs both being accepted.

I agree with all your closings.

Clorr commented 6 years ago

Ok, thanks. For #251, #260, #252, I think they are hot enough so if they stay a bit more, it won't be a big problem. Anyhow, I will possibly have also conflict so merging "right now" will likely take one or 2 days...

iperov commented 6 years ago

Merge GAN first please.

bryanlyon commented 6 years ago

I agree that #217 Should be the first merge. It's a major update and will actually require rewriting many of the other PRs since it includes an architectural change.

iperov commented 6 years ago

@Clorr where is batch size fix ? https://github.com/deepfakes/faceswap/pull/236 looks like batch fix in Original trainer has been erased :(

Clorr commented 6 years ago

236 has been merged. However due to structure changes it may have been lost. I'm on my phone so I can't check, but feel free to review and push a pr if necessary

iperov commented 6 years ago

yes man who changed structure didnt transfer content of Trainer I so mad.

iperov commented 6 years ago

push a pr if necessary no thx, I so tired. Working in opensource much exhausting me. Its require to check all pr's, because ppl can erase something I wrote before as it happened with #236 and #259 Also https://github.com/deepfakes/faceswap/pull/253 man cannot provide proofs why his improve can detect more faces.

Clorr commented 6 years ago

It exhausts you because you put too much passion and expectations in the project. We don't care if this project fails here or here sometime, it is not rocket science or nuclear research.

It is just a collective effort to share some knowledge and if there is a defect someone who is bothered by the problem will fix it.

Some attention has to be given so that it does not go too bad though, but we are not perfect