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

Compile Release Transform: raise MissingDateKeyError('The date field of at least one release is missing.') #279

Closed
odscjames opened this issue Apr 14, 2020 · 11 comments
Labels
steps Relating to specific steps (transforms)
Milestone

Comments

@odscjames
Copy link

We should at least make sure such errors don't crash the whole Python process - but how should the app handle data with no date?

@jpmckinney jpmckinney added the steps Relating to specific steps (transforms) label Apr 14, 2020
@jpmckinney
Copy link
Member

What do you think the behavior should be, @yolile @romifz @mrshll1001?

The problem is that, if the date is missing, then the release can't be ordered correctly for the merge routine.

@romifz
Copy link

romifz commented Apr 15, 2020

I think these releases could be ignored. Are transformation warnings reported somewhere? It would be nice to have a place to look for these.

@jpmckinney
Copy link
Member

For your question, that's the subject of #242

@jpmckinney jpmckinney added this to the V1 milestone Apr 20, 2020
@odscjames
Copy link
Author

@odscjames
Copy link
Author

As this is causing crashes, I'm going to do a quick fix under the small work rule. I'll just get it to drop whole OCID's if "date" is missing with a note. This is a bit of an extreme option but at least it won't be crashing and other data will be processed.

@odscjames
Copy link
Author

Deployed quick fix. Leaving open to work out if better way to process?

@jpmckinney
Copy link
Member

jpmckinney commented Apr 22, 2020

@romifz Does it make sense to not produce any compiled release for an OCID, if any of its individual releases lacks a date? Or is it preferable to create a compiled release with the individual releases that have a date? I don't have a strong opinion; it's a choice between missing processes and missing releases.

cc @duncandewhurst as not tagged earlier due to leave.

@romifz
Copy link

romifz commented Apr 22, 2020

I think the second option may be better. When doing a review we want to have as many data available as possible. If we exclude processes there is a small chance of missing an issue we can report on.

@duncandewhurst
Copy link

I think the second option may be better. When doing a review we want to have as many data available as possible. If we exclude processes there is a small chance of missing an issue we can report on.

Agreed. We should also add a step to our standard notebook set up to check for these errors. I've added a note to crm issue #3918

@odscjames
Copy link
Author

Looking at this. Still good to do quick fix tho, as that will catch any errors OCDS Merge throws, not just date ones.

odscjames added a commit that referenced this issue Apr 23, 2020
odscjames added a commit that referenced this issue Apr 23, 2020
odscjames added a commit that referenced this issue Apr 24, 2020
…ases without dates and warn

Tweak logic, as reviewed in pull request.

#279
#284
@odscjames
Copy link
Author

Deployed new process - will merge with date ones and alert. Think this is now done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
steps Relating to specific steps (transforms)
Projects
None yet
Development

No branches or pull requests

4 participants