A much simpler migration script that relies on spacepy - #1518
A much simpler migration script that relies on spacepy#1518vineetbansal wants to merge 12 commits into
Conversation
| if "Parents" in cdf.attrs: | ||
| parents = [str(p) for p in cdf.attrs["Parents"]] | ||
| cdf.attrs["Parents"] = [make_new_file_name(p, major) for p in parents] | ||
|
|
There was a problem hiding this comment.
Per Tim's comment, it would be nice to update the File_naming_convention attribute to source_descriptor_datatype_yyyyMMdd_vMMM.mmmm as well.
There was a problem hiding this comment.
I pointed out on the ticket for this that at least one attribute is missing: File_naming_convention: source_descriptor_datatype_yyyyMMdd_vNNN. That is from the global cdf attributes yaml. I don't know if anyone has scrubbed products for other possible global attributes that need to be updated.
There was a problem hiding this comment.
Ah - got it now. That file looked like a readme of sorts so I missed the fact that its an actual attribute mapping yaml. Done.
There was a problem hiding this comment.
I did a quick check and didn't see any other missing globals.
|
I haven't looked at code details. I have couple general questions.
|
tech3371
left a comment
There was a problem hiding this comment.
Can you overwrite previous script to reduce code to review and to see what we are doing differently this time? or comment which ones we have changed? This will help me a lot since lot of context switching is happening.
|
@tech3371 - the problem with using Unless we're willing to take the time to figure out which files are affected and which are okay, the simpler option is to pretend that the previous renaming never happened and do it again using this script, and overwrite the previously generated |
|
@tech3371 - I don't think code redundancy should be a priority right now. We can do that once we're past this release. I'd keep the old scripts for reference. The conceptual difference between the old approach and the new approach are:
All the bash scripts are driving scripts - the function you want to read is At the end of the day all we're doing is executing a function ( |
|
Note from the meeting today, check for accuracy: because some of the filenames changed in the old script (the date, not just the version), we will need to compare the newly-renamed files against the currently-renamed files in production, and see where there are differences. Any files that are in production but not in the new set of files will need to be removed from the bucket and the index (after the renaming, the reindexer was run and would have ingested those as "new" files). |
tmplummer
left a comment
There was a problem hiding this comment.
They python script looks good to me with one suggested change. It would be good to get @bryan-harter to review the AWS related files.
Co-authored-by: Tim Plummer <timothy.plummer@lasp.colorado.edu>
tmplummer
left a comment
There was a problem hiding this comment.
Should the initial migrate code be removed as part of this PR?
|
Yes - @tech3371 mentioned this earlier. I'll merge everything into the |
|
hmm - I had no idea ancillaries were not getting renamed. I guess no one else here did either. I think it might be good to make this change (though I don't understand why this regex matches science files only and excludes ancillary files - someone else will need to confirm that). Bryan will have to decide whether to leave mag science files as-is or re-rerun the renaming on them after making this fix, or if fixing those can be a task folded in with the exercise of fixing the |
|
Good catch. On our side, we forgot to think about the possibility that ancillary files would have the Ancillary files have a different number of underscore-delimited sections in their filename because they do not have the data level section. |
|
@vineetbansal The regex separates them structurally. Science names carry the data level as its own underscore-delimited token ( Also verified empirically, not just by inspection: I ran this exact gate over the |
|
makes sense..you might want to walk to Bryan's office to discuss whether he's ok rerunning this for mag after merge since he may not be following this thread closely and we're out of time. |
|
I work from home unfortunately. @bryan-harter please weigh in here? |
|
I see this now! I can work on this in a bit. |
|
Ok, I believe I fixed them all. Would you mind trying again? |
|
@bryan-harter I wasn't sure if you were talking to me or Vineet, but I went ahead and had Fable 5 re-run the audit script from my comment in imap_processing#3348 and here's what it came back with:
|
|
@vineetbansal are you planning on incorporating my suggested fix to your |
fix to avoid picking up ancillary files (which were not renamed) in the Parents for science files, and thus avoid migrating those entries. Co-authored-by: Shawn Polson <sapols@yahoo.com>
|
@sapols - I incorporated your suggested fix (through the UI) but it seems partial - And now the whole suggestion has disappeared from github anyway. Do you mind doing your AI magic to suggest the fix? I'm kinda new to this seemingly partial agentic integration in github. EDIT: I see it now. I'll make the change. |
Add regex constant missing from last commit
|
Ah sorry for the confusion there @vineetbansal. Fable helped write the regex but it was my call to use GitHub's suggestion feature that way. Totally see how splitting the code between suggestion and comment wasn't obvious. Glad you figured it out 😅 Looks good now |
|
Just a general comment, we have now already used this script to alter/rename all of the files that we need to, and I don't think it is super likely that we will need this script again in the future. Are there thoughts about keeping it in the repo v.s. not keeping it? I don't mind either way. But perhaps there are reasons to keep the repo "clean" (reducing AI agent context windows?) |
|
I'll also approve it for now, and feel free to merge it so we can keep our open PRs down! It shouldn't really affect things either way. |
Closes #1414
Similar in operational details as a previous PR, but now we simply open the cdf through
spacepy, modifyData_version,Logical_file_id, andParents, and writes it out usingspacepy. Similar to what Menlo did on their end.There's no multiprocessing/multihreading in the script (
spacepyis crashing on multiprocessing), but we can still partition the runs bySRC_PREFIX(the paths relative to the bucket root that we want to process).We should probably do this in parallel for each instrument, something like: