-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor codebase for new euclid workflow usage with tests #180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Toyosi,
Gonna preface this that I dont really know kade or bajor that well, so this PR review isnt up to snuff of what I usually review. But I think that is ok knowing that the goal is to abstract a bit to get kade working for one other project. And as we find more use cases we can abstract and add more.
I had a couple questions, but also knowing that you and @lcjohnso have been in more discussion about the code changes than I have, if you both to decide to move forward, I'm ok with it .
@@ -0,0 +1,5 @@ | |||
class AddModuleNameToContext < ActiveRecord::Migration[7.0] | |||
def change | |||
add_column :contexts, :module_name, :string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these migrations, are you planning on backfilling the module_name and extractor_name for any current contexts in production? (Right now there is only 1 context, which if I think about it makes sense, since only 1 project has been using this so far 😆 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, plan is to use galaxy_zoo
and cosmic_dawn
or in this new case euclid
for the context module_name
and extractor_name
respectively. We have just one context on the db currently so the aim is to update that after migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do be sure to document and execute this data backfill!
…exises for extractor tests, remove error on baseclass
Changes for new euclid workflow
What was done:
finder.rb
base_extractor
classcosmic_dawn_and_euclid
classdecals
(the initial workflow configured) extractor class to inherit from the basecosmic_dawn
extractor class to inherit from shared class with euclideuclid
extractor classZoobot.rb
:label_column_headers
dynamiccreate_job
:training_data
:Schedule.yml
:bajor/client
: