refactor namespaces in criterion interface

[erip]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
• Did you read the contributor guideline?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

What does this PR do?

Fixes #1672 in part (part 1: context)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[facebook-github-bot]

@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[myleott]

I'm almost done with this one, hoping to merge later today :) I had to make some changes to support backwards compatibility, since there's a lot of internal criterions that still use the old API.

[erip]

Sorry for all the extra work, but I'm excited about these changes. Wiring up hydra should be a breeze after this.

[myleott]

Sorry for the delay, still iterating on this. Will be merged soon...

[facebook-github-bot]

@myleott is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[erip]

Woo! Glad to see this land -- thanks, @myleott. I see that this was a lot of work -- if there's anything you want to push back to me for the other components, please don't hesitate.

[myleott]

Yeah thanks for all your work getting this started!

The first one took a bit longer because it required some discussion and figuring out the right way to maintain backward compatibility. The key is providing a Legacy version of the base classes, for example the LegacyFairseqCriterion.

It’d be super helpful if you can add some of those for the other PRs, but in general I hope the next few of these should be easier to merge.

[erip]

Awesome. Happy to do that - looks like a light lift after the hard decision making was made up front. 😄 Should I also make PRs to pytorch/translate in tandem (like the associated commit w/ this PR)? I was given the impression that translate and fairseq were going to merge eventually, but I don't suspect that changes the need to keep them in lock step.

[MultiPath]

Hi, what is the purpose for this change which might cause all the user defined criterion broken?

[erip]

The purpose of this change is to divorce components in fairseq from argparsing. If you want to use some model/criterion/task/etc. outside of fairseq, you basically cannot. We have retained a LegacyFairseqCriterion interface which is a minimal change to keep compatibility with the previous interface. It doesn't offer perfect backwards compatibility, but fairseq isn't 1.0.0 yet so the API is subject to change some small amount.

[MultiPath]

I don't understand.
The original criterion design already contained "add_args" functions to add specific arguments. I am not sure the current change for "from_args" is necessary and I felt it made code even confusing.

[erip]

The original criterion design stored an argparse.Namespace as an instance variable which means that you need one to create a criterion which is inconvenient. Eventually all of these {add,from}_args methods will be replaced by proper configuration.

[MultiPath]

Not sure what do you mean by "you need one to create a criterion which is inconvenient. "
I think the design of args has more freedom to add different configurations as needed.

[erip]

Basically:

class Criterion:
    def __init__(self, args: argparse.Namespace):
        self.args = args # <-- this has nothing to do with what it means to be a criterion

The above code is very nasty and makes the code very hard to test, very hard to use in production situations, and very brittle.

[MultiPath]

Ok.. If it is the case, everything needs to be changed in fairseq...

[erip]

These changes are not as dramatic as they appear. Basically it's standardizing the code toward best practices which has a lot of benefit downstream in testing and consuming.

[MultiPath]

I think using "args" makes it much easier to pass arguments which might be defined outside of criterions.

[erip]

Why can't you pass it to the constructor directly?

[MultiPath]

Also, if a function requires 10+ more arguments as inputs to "init", it will also be very difficult to read and work with.

[erip]

Difficult to read: maybe. Consider abstracting something.
Difficult to work with: I don't think I agree.

[MultiPath]

Anyway, I still don't think it is a good idea to remove all "args" to build the current model/criterion/etc.
There may be a lot of places where you need to have this "args" as input, otherwise it is easily to get 10+ or 20+ inputs in a complicated model.
Just to be clear, since Myle has already accepted the PR, I just left my comments here.

[erip]

To make this exceeding clear: if someone develops an interesting optimizer in fairseq that I want to use in my pytorch code, there's currently no way to do that unless I mock argument parsing. Optimizers should not care about argument parsing much like models, lr schedulers, tokenizers, byte-pair encoders, loss functions, or just about any other component in software (outside of a main function) should not.

[MultiPath]

..but this PR is about "loss functions".

[erip]

I was giving an example. There are PRs for every other component, as well. See #1743, #1733, #1732, #1731, #1730

[MultiPath]

Yes, I understand.
I think it is super unnecessary and makes everything hard to read, change, and maintain our previous codebase. Especially for "tasks", "models".

[myleott]

Sorry just catching up on this.

@MultiPath, I shared some context/motivation in the other PR (#1743), but I agree that we need to be careful here for several reasons:

1. Currently fairseq uses args everywhere, so we need to be careful not to break existing code. We introduced the "LegacyFairseqCriterion" base class that should be a drop-in replacement to make any existing code continue to work.
2. There are cases where using a single args is very helpful and we should continue to support this where it's appropriate.
3. The hope is that this will make it easier to use fairseq as a library. The motivation is to let people initialize and use fairseq components as standalone pieces in other projects, rather than only supporting the fairseq command-line usage.

[myleott]

The original criterion design already contained "add_args" functions to add specific arguments. I am not sure the current change for "from_args" is necessary

Hmm, seems this is a github bug. Please ignore the "Files Changed" shown in this PR. The actual commit is 46b773a, which is different from what's shown in this PR. Notably, we didn't end up adding from_args and also introduced LegacyFairseqCriterion for backward compatibility.