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

Configure warnings #319

Merged
merged 9 commits into from
Sep 12, 2017
Merged

Conversation

drdavella
Copy link
Contributor

@drdavella drdavella commented Aug 25, 2017

This PR does two things:

  1. It makes version mismatch warnings silent by default
  2. It enables warnings for unrecognized tags to be silenced

@nden, this seems like the right way to handle the large number of version mismatch warnings in jwst in the long term. However, I think that unrecognized tag warnings should be enabled by default, and there are a few other more serious warnings which are still not possible to silence at all.

Also, while this resolves #313 for the most part, it does not add a module-level configuration mechanism. There are too many questions about precedence that make it a little too complicated to implement without sufficient motivation. Please let me know if you disagree.

@drdavella drdavella requested a review from nden August 25, 2017 20:30
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 94.595% when pulling 5aec392 on drdavella:configure-warnings into a23105a on spacetelescope:master.

@drdavella drdavella added this to the 1.3.0 milestone Sep 1, 2017
@drdavella
Copy link
Contributor Author

@nden, despite the appearance of the diff, this is a pretty small change. I'd like to merge it if you have no objections. It should not cause any more warnings to be produced by the JWST pipeline.

@nden
Copy link
Contributor

nden commented Sep 8, 2017

@drdavella This looks good to me. I have one suggestion.
Would it be better to update *Type classes to include the supported_versions attribute you added in #284. I've added supported_versions to transform types and I don't see warnings any more. I think it's good to explicitly state which versions are supported and then warnings will be issued for those that are not.

If so, the default of ignore_version_mismatch can be set to False and let application choose how to deal with warnings. The reasoning is that sometimes the warning may point to a real problem.

If you agree, just set the default of ignore_version_mismatch to False in this PR. Since I've added supported_versions to experiment I can make a PR for that.

@drdavella
Copy link
Contributor Author

drdavella commented Sep 8, 2017

@nden I've been trying to clarify my thoughts on this issue. I think I need to add some documentation that describes these changes and the philosophy behind them. In the meantime, here are my current thoughts (which maybe can eventually be converted into documentation in some form):

There are really two different stages at which warnings can occur:

  • schema validation, and
  • tag conversion

Also, tag classes now have two different attributes related to versioning:

  • version, and
  • supported_versions

There are also now two levels of warning control:

  • ignore_version_mismatch which is set to True by default, and
  • ignore_unrecognized_tag, which is set to False by default

The version attribute of a tag class describes the current (i.e. latest) version of that tag. It should correspond to the version of the latest available schema for that tag. For example, if we have a schema that defines foo-3.2.0, then the version attribute of the FooType class will be "3.2.0".

During schema validation, we detect mismatched tag versions. For example, if I read an ASDF file that references foo-3.0.0 or foo-3.3.0 but the latest schema is foo-3.2.0, then a version mismatch warning will occur. This has nothing to do with the tag conversion stage: this is strictly a statement about schema validation. This kind of warning is now controlled by ignore_version_mismatch, and it is currently ignored by default since it does not indicate a serious problem.

The supported_versions attribute is used to describe the versions of a tag that can be successfully translated by a particular tag class. By default, it is assumed that all versions of a particular tag can be converted by the corresponding tag class. However, this is not always true. For example, if foo-3.0.0 introduced backwards-incompatible changes, our tag class may only be able to convert version 3.0.0 and later. We can declare this by setting supported_versions=AsdfSpec('>=3.0.0').

During tag conversion, we determine whether a particular schema can be successfully converted to a custom Python data structure by a particular tag implementation. Continuing with the example above, if we encounter foo-3.0.0, our supported_versions attribute tells us we can successfully convert it, so there is no warning, even though the latest version we know of is foo-3.2.0. However, if we encounter foo-2.7.1, we can't convert it, so there will be a warning. This warning can't be ignored at all.

The ignore_unrecognized_tag option controls warnings that occur when no corresponding tag class can be found for a particular tag. For example, if I read an ASDF file that makes reference to baz-1.2.0, but I don't have a BazType class, then this warning will occur. These warnings are not suppressed by default.

TL;DR: It is useful and a good idea to explicitly define supported_versions for tag classes (including transform types), but it is not strictly necessary in most cases. However, since supported_versions has no bearing on schema mismatch warnings, and since these warnings indicate a relatively minor issue, it seems reasonable to ignore them by default.

This is to make sure that warnings occur even when there are tag classes
for a particular tag, as long as that tag does not represent the latest
known version of that tag. The purpose of this commit is to prevent
`supported_versions` from having an affect on YAML validation warnings.

This also refactors the warning handling logic to consolidate and reuse
code.
@drdavella
Copy link
Contributor Author

@nden reported that the use of supported_versions affected the behavior of YAML validation warnings, which is not what I intended. This has now been fixed and I believe that it is ready to be merged as soon as the tests pass.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 94.904% when pulling ff9ec0c on drdavella:configure-warnings into a23105a on spacetelescope:master.

@nden
Copy link
Contributor

nden commented Sep 12, 2017

OK, so really the version_mismatch warning is of no interest to end users. I think this can merged. I'll open a separate issue about supported_versions as I see some potential issues.

@drdavella
Copy link
Contributor Author

drdavella commented Sep 12, 2017

I'm not sure it will never be of use to end users, but I think the warnings are of a sufficiently minor nature that they should be have to be explicitly enabled (maybe think of it like -Wall for a C compiler, although it's probably not a great analogy).

@drdavella drdavella merged commit 2053a44 into asdf-format:master Sep 12, 2017
@drdavella drdavella deleted the configure-warnings branch September 12, 2017 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mechanism for configuring warning levels
3 participants