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

refactor codebase for new euclid workflow usage with tests #180

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

Tooyosi
Copy link
Contributor

@Tooyosi Tooyosi commented Oct 28, 2024

Changes for new euclid workflow

What was done:

  • Refactor codebase to be less specific to the current cosmic dawn work flow
    • Changes here made on:
    • Label Extractors:
      • finder.rb
      • Create new base_extractor class
      • Create shared cosmic_dawn_and_euclid class
      • Modify decals (the initial workflow configured) extractor class to inherit from the base
      • Modify cosmic_dawn extractor class to inherit from shared class with euclid
      • Create new euclid extractor class
    • Zoobot.rb:
      • Make label_column_headers dynamic
    • create_job:
      • Pass in extractor name to bajor client so it knows what workflow the job belongs to
    • training_data:
      • Fetch the Module(in this case galazy_zoo) and extractor name to generate the proper label_column
    • Schedule.yml:
      • Add new cron job for euclid workflow training
    • bajor/client:
      • pass new options to the request body to match changes made on bajor (pr)

Copy link

@yuenmichelle1 yuenmichelle1 left a 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 .

app/modules/label_extractors/base_extractor.rb Outdated Show resolved Hide resolved
app/modules/label_extractors/base_extractor.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
class AddModuleNameToContext < ActiveRecord::Migration[7.0]
def change
add_column :contexts, :module_name, :string

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 😆 )

Copy link
Contributor Author

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

Copy link
Member

@lcjohnso lcjohnso Nov 8, 2024

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!

lib/bajor/client.rb Outdated Show resolved Hide resolved
spec/modules/label_extractors/galaxy_zoo/euclid_spec.rb Outdated Show resolved Hide resolved
@lcjohnso lcjohnso removed the request for review from zwolf November 8, 2024 15:34
@Tooyosi Tooyosi merged commit 0b691cc into main Nov 11, 2024
1 check passed
@Tooyosi Tooyosi deleted the euclid-workflow-training branch November 11, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants