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

Deprecate "image_type" in Crop, Slice and CropMirrorNormalize #2061

Merged
merged 7 commits into from
Jun 25, 2020

Conversation

jantonguirao
Copy link
Contributor

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

Pick one, remove the rest

  • It deprecates the argument "image_type" in Crop, Slice and CropMirrorNormalize, as it is not used

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    Removed "image_type" from the schema of the mentioned operators
    Added a deprecation warning for using "image_type" with those operators
    Adjusted examples
  • Affected modules and functionalities:
    Operators Crop, Slice and CropMirrorNormalize
  • Key points relevant for the review:
    N/A
  • Validation and testing:
    Test added
  • Documentation (including examples):
    N/A

JIRA TASK: [Use DALI-XXXX or NA]

@jantonguirao jantonguirao requested a review from klecki June 25, 2020 05:46
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@jantonguirao
Copy link
Contributor Author

!build

Comment on lines 422 to 429
if "image_type" in kwargs.keys() and name in ("Crop", "Slice", "CropMirrorNormalize"):
with warnings.catch_warnings():
warnings.simplefilter("default")
warnings.warn(
("Argument name 'image_type' for operator {} is deprecated. " +
"It should be removed from the argument list, as it is not necessary.").format(type(self).__name__),
DeprecationWarning, stacklevel=2)
del kwargs["image_type"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this kind of specific warning being placed in generic place. I think that a mechanism for deprecation at schema level would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a general solution

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422527]: BUILD STARTED

@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422710]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422710]: BUILD FAILED

@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422730]: BUILD STARTED

@@ -47,6 +47,12 @@ struct DefaultedArgumentDef {
Value *default_value;
};

struct DeprecatedArgumentDef {
std::string deprecated_str;
Copy link
Contributor

@mzient mzient Jun 25, 2020

Choose a reason for hiding this comment

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

Q: How about just printing this in Python, too? (instead of renamed_to/ignore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed_to/ignore are not only for printing but also to know whether to automatically redirect the deprecated argument to the new name, or totally remove it from the arguments (of course while warning the user that he/she is using a deprecated argument)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422730]: BUILD FAILED

new_name = self._schema.DeprecatedArgRenamedTo(arg_name)
ignore = self._schema.DeprecatedArgIgnore(arg_name)
if new_name:
msg += " Use \"{}\" instead.".format(new_name)
Copy link
Contributor

@mzient mzient Jun 25, 2020

Choose a reason for hiding this comment

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

  1. This will produce a leading space in case of empty message.
  2. When the message does not end in full stop, it will look awkward (a new line would be better)
Suggested change
msg += " Use \"{}\" instead.".format(new_name)
if msg: msg += "\n"
msg += "Use \"{}\" instead.".format(new_name)

or

Suggested change
msg += " Use \"{}\" instead.".format(new_name)
if msg:
msg += " " if msg.endswith('.') else ". "
msg += "Use \"{}\" instead.".format(new_name)

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 am assuming that the message is not empty and terminated with ".". That being said, I'll add your suggestion

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1422894]: BUILD STARTED

Comment on lines 463 to 469
DLL_PUBLIC inline OpSchema &AddDeprecatedArg(const std::string& arg_name,
const std::string& deprecation_str,
const std::string& renamed_to,
bool ignore = true) {
deprecated_arguments_[arg_name] = {deprecation_str, renamed_to, ignore};
return *this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can add overloads:

AddDeprecatedArg(arg_name, bool ignore=false, optional_message="")
AddDeprecatedArg(arg_name, replaced_by, bool ignore=true, optional_message="");

I would add a doc that one is for removing arguments and the other for replacing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe DeprecateArg instead?

@@ -178,7 +177,7 @@
" BATCH_SIZE, device_id=i, shard_id=i, num_shards=NUM_DEVICES),\n",
" batch_size=BATCH_SIZE,\n",
" output_shapes=shapes,\n",
" output_dtypes=dtypes,\n",
" dtypes=dtypes,\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is specification of dataset, it uses output_dtypes (to mimic the TF api).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I thought I skipped those but in this one I changed it by mistake. Good catch

Comment on lines 658 to 684
DLL_PUBLIC inline bool IsDeprecatedArg(const std::string& arg_name) const {
return deprecated_arguments_.find(arg_name) != deprecated_arguments_.end();
}

DLL_PUBLIC inline std::string DeprecatedArgMsg(const std::string& arg_name) const {
auto it = deprecated_arguments_.find(arg_name);
DALI_ENFORCE(it != deprecated_arguments_.end(),
make_string("Argument \"", arg_name, "\" is not marked as a deprecated argument"));
if (!it->second.deprecated_str.empty())
return it->second.deprecated_str;
return make_string("Argument \"", arg_name, "\" for operator \"", name_,
"\" is now deprecated.");
}

DLL_PUBLIC inline std::string DeprecatedArgRenamedTo(const std::string& arg_name) const {
auto it = deprecated_arguments_.find(arg_name);
DALI_ENFORCE(it != deprecated_arguments_.end(),
make_string("Argument \"", arg_name, "\" is not marked as a deprecated argument"));
return it->second.renamed_to;
}

DLL_PUBLIC inline bool DeprecatedArgIgnore(const std::string& arg_name) const {
auto it = deprecated_arguments_.find(arg_name);
DALI_ENFORCE(it != deprecated_arguments_.end(),
make_string("Argument \"", arg_name, "\" is not marked as a deprecated argument"));
return it->second.ignore;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can consider taking a look at what @JanuszL did recently with the oprator metadata and return this as a dictionary from one function call.

Comment on lines 668 to 669
return make_string("Argument \"", arg_name, "\" for operator \"", name_,
"\" is now deprecated.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how I feel about the default here, but at least it fixes a bug with warning about deprecation of output_dtype in an op that didn't use it.

Maybe mention it in the doc for AddDeprecatedArg?

Comment on lines 420 to 423
del kwargs[arg_name]
elif ignore:
msg += " The argument is no longer used and should be removed."
del kwargs[arg_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

If ignore means to not error, than it should be:

Suggested change
del kwargs[arg_name]
elif ignore:
msg += " The argument is no longer used and should be removed."
del kwargs[arg_name]
if ignore:
del kwargs[arg_name]

Also you have some part of the error message defaulted here, which makes it two different places.

Copy link
Contributor

@mzient mzient Jun 25, 2020

Choose a reason for hiding this comment

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

AFAIK ignore means "no longer used"

@@ -1619,9 +1622,20 @@ def test_output_dtype_deprecation():
# Verify DeprecationWarning
assert len(w) == 1
assert issubclass(w[-1].category, DeprecationWarning)
assert ("Argument name 'output_dtype' for operator CropMirrorNormalize is deprecated. " +
"Use 'dtype' instead.") == str(w[-1].message)
assert ('Argument "output_dtype" for operator "CropMirrorNormalize" is now deprecated. ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

I used the ' as this is what Python is using, but I don't now how it goes with rest of DALI.

There is image_type specified and it was also deprecated, so this test will fail I think.

if msg:
msg += " " if msg.endswith('.') else ". "
msg += "Use \"{}\" instead.".format(new_name)
kwargs[new_name] = kwargs[arg_name]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you check if both were specified and raise the TypeError as previously?

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

Signed-off-by: Joaquin Anton <janton@nvidia.com>
Comment on lines 463 to 472
/**
* @brief Marks an argument as deprecated in favor of a new argument
* @remarks There are three ways to deprecate an argument
* 1. renamed_to provided, means the argument has been renamed and we can safely
* propagate the value to the new argument name.
* 2. renamed_to not provided and ignore==true, means the operator will not use the
* argument at all and it can be safely discarded.
* 3. renamed_to not provided and ignore==false, means the operator will still use the
* deprecated argument until it is finally removed completely from the schema.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @brief Marks an argument as deprecated in favor of a new argument
* @remarks There are three ways to deprecate an argument
* 1. renamed_to provided, means the argument has been renamed and we can safely
* propagate the value to the new argument name.
* 2. renamed_to not provided and ignore==true, means the operator will not use the
* argument at all and it can be safely discarded.
* 3. renamed_to not provided and ignore==false, means the operator will still use the
* deprecated argument until it is finally removed completely from the schema.
*/
/**
* @brief Marks an argument as deprecated in favor of a new argument
*
* Providing renamed_to means the argument has been renamed and we can safely
* propagate the value to the new argument name.
*/

Comment on lines 482 to 491
/**
* @brief Marks an argument as deprecated in favor of a new argument
* @remarks There are three ways to deprecate an argument
* 1. renamed_to provided, means the argument has been renamed and we can safely
* propagate the value to the new argument name.
* 2. renamed_to not provided and ignore==true, means the operator will not use the
* argument at all and it can be safely discarded.
* 3. renamed_to not provided and ignore==false, means the operator will still use the
* deprecated argument until it is finally removed completely from the schema.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* @brief Marks an argument as deprecated in favor of a new argument
* @remarks There are three ways to deprecate an argument
* 1. renamed_to provided, means the argument has been renamed and we can safely
* propagate the value to the new argument name.
* 2. renamed_to not provided and ignore==true, means the operator will not use the
* argument at all and it can be safely discarded.
* 3. renamed_to not provided and ignore==false, means the operator will still use the
* deprecated argument until it is finally removed completely from the schema.
*/
/**
* @brief Marks an argument as deprecated
* @remarks There are three ways to deprecate an argument
* 1. removed==true, means the operator will not use the
* argument at all and it can be safely discarded.
* 2. removed==false, means the operator will still use the
* deprecated argument until it is finally removed completely from the schema.
* 3. For renaming the argument see DeprecateArgInFavorOf
*/

msg = meta['msg']
if new_name:
if new_name in kwargs:
raise TypeError("Operator {} got an unexpected 'output_dtype' argument when 'dtype' is already provided".format(type(self).__name__))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise TypeError("Operator {} got an unexpected 'output_dtype' argument when 'dtype' is already provided".format(type(self).__name__))
raise TypeError("Operator {} got an unexpected '{}' deprecated argument when '{}' is already provided".format(type(self).__name__, arg_name, new_name))

raise TypeError("Operator {} got an unexpected 'output_dtype' argument when 'dtype' is already provided".format(type(self).__name__))
if msg:
msg += " " if msg.endswith('.') else ". "
msg += "Use \'{}\' instead.".format(new_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part of the string can be already stored upon creation the deprecated metadata.

kwargs[new_name] = kwargs[arg_name]
del kwargs[arg_name]
elif removed:
msg += " The argument is no longer used and should be removed."
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this. I think it would be better if we can see the deprecation message being constructed in one place.

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1423311]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1423311]: BUILD PASSED

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.

4 participants