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

Remove soon-to-be-deprecated usage of AsdfFile.open #2814

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

drdavella
Copy link
Collaborator

@drdavella drdavella commented Oct 31, 2018

This will take effect once asdf-format/asdf#579 is merged, but there's no harm in making this change now.

This closes #2795.

Copy link
Collaborator

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

This looks good. The clarifying variable names in model_base.py are good too.

Thanks!

@nden nden added this to the 0.13.0 milestone Nov 2, 2018
@drdavella
Copy link
Collaborator Author

Seems like the Jenkins job is hung...

@jaytmiller jaytmiller closed this Nov 7, 2018
@jaytmiller jaytmiller reopened this Nov 7, 2018
@drdavella
Copy link
Collaborator Author

Still failing. Is it just me or is there a problem with Jenkins?

@jdavies-st jdavies-st closed this Nov 7, 2018
@jdavies-st jdavies-st reopened this Nov 7, 2018
@jdavies-st
Copy link
Collaborator

Yes, Jenkins was hosed. Matt hit the shiny red button to restart. I've restarted (I think the Jenkins CI test by closing and reopening this PR).

@stscieisenhamer & @nden, any idea what is going on with this failure in the TravisCI build?

https://travis-ci.org/spacetelescope/jwst/jobs/452031031#L1496

Does a gwcs object no longer have a pipeline attribute?

@nden
Copy link
Collaborator

nden commented Nov 7, 2018

The wcs tags were removed from asdf as they are now (and have been for quite a while) in gwcs. Something must be using an old tag or it's a bug. I'll investigate.

@drdavella
Copy link
Collaborator Author

drdavella commented Nov 8, 2018

I think the problem is files in jwst/exp_to_source/tests/data that contain references to !wcs/.... We just removed the old WCS tags and schemas from ASDF, so these tags won't resolve anymore.

I suspect that a simple search and replace of these for the corresponding tag name in gwcs should fix the problem. For example, !wcs/step-1.0.0 should become !<tag:stsci.edu:gwcs/step-1.0.0>.

@jdavies-st
Copy link
Collaborator

I see. Does this mean that all old data like this will now fail to to have a functioning GWCS object without it having to be rebuilt using a current version of asdf and gwcs?

@drdavella
Copy link
Collaborator Author

@jdavies-st the short answer is yes. Of course, you could grab an older version of asdf, but that's not ideal, is it?

@nden we made this change expecting that there would be no residual uses of ASDF WCS, but maybe we should go with a deprecation period instead.

We're in a tricky place here because if we don't try to be diligent about cleaning these things up, ASDF is going to become a giant crapball of spaghetti code that mostly supports legacy uses. We should handle this better going forward, but I don't think we were expecting this particular change to have any impact.

Unfortunately, making this change has exposed some other issues that will need to be addressed (see #2849).

Considering the hour, I think the best course of action is probably to sit down with a tall scotch and forget all about of this for a little while.

@stscieisenhamer
Copy link
Collaborator

hold it...wait...i'm in the wrong conversations....

@philhodge
Copy link
Contributor

Is it (theoretically) possible to edit the ASDF extension to change the tags, as an alternative to rebuilding the extension?

@drdavella
Copy link
Collaborator Author

@philhodge if I understand correctly, it's probably possible in theory, but I'm not sure we want to support this for the general case since it will probably be fairly difficult to maintain. It would probably be easier just to write a one-off script to convert files like these, but at that point it might just be easier to regenerate them.

@jdavies-st
Copy link
Collaborator

jdavies-st commented Nov 9, 2018

@drdavella, can you rebase this to jwst/master? Tests should pass if you do.

@drdavella
Copy link
Collaborator Author

Looks like we're good now!

@jdavies-st
Copy link
Collaborator

Thanks @drdavella!

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.

AsdfFile.open will be deprecated
6 participants