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

Persist run options & set at creation (not claim) #2085

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

taylordowns2000
Copy link
Member

@taylordowns2000 taylordowns2000 commented May 14, 2024

Requesting guidance before going further. Does this feel like the right way to address the run options issue?

Required context (pre-reading) to review:

Note that if we like this idea, next we still need to:

  1. remove the "get options" bit that happens during claim
  2. ensure that these options are delivered properly via the channel to the worker
  3. check to see which options are set by the project admin (e.g., enable dataclip storage) and which options are set by the instance administrator (e.g., max run duration)

Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.09%. Comparing base (ae29143) to head (b467ca6).
Report is 7 commits behind head on main.

Current head b467ca6 differs from pull request most recent head 281c00d

Please upload reports for the commit 281c00d to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2085   +/-   ##
=======================================
  Coverage   90.09%   90.09%           
=======================================
  Files         268      268           
  Lines        8951     8955    +4     
=======================================
+ Hits         8064     8068    +4     
  Misses        887      887           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@taylordowns2000
Copy link
Member Author

@stuartc , please take a look here! context in the PR itself

@elias-ba
Copy link
Contributor

@taylordowns2000 this looks like a valid way to implement this. I am not sure I can't validate it's relevancy but it seems to me as a better way of dynamically calculating the run options. Maybe wait for @stuartc to confirm that point.

Copy link
Contributor

@elias-ba elias-ba left a comment

Choose a reason for hiding this comment

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

@taylordowns2000 this implementation looks good to me (assuming that tests and other bits will come later)

Copy link
Member

@stuartc stuartc 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, thanks for the detailed PR.

My only comment here is that I think we should make the run options more than a type. Right now it's defined in RunWithOptions.

I think is should become an embedded schema, so using Ecto.Schema.

And then in the Run model, it becomes embeds_one :options, Run.Option

This will require some light refactoring on get_run_options on UsageLimiter on both Lightning and Thunderbolt.

I believe this change will give the run options more prominence and make 'just dumping stuff in' less of a temptation.

@stuartc
Copy link
Member

stuartc commented May 23, 2024

@taylordowns2000 this implementation looks good to me (assuming that tests and other bits will come later)

Oh and yes, @taylordowns2000 - tests please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New Issues
Development

Successfully merging this pull request may close these issues.

None yet

3 participants