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

API/DOC/backwards: rework config defaults #125

Merged
merged 61 commits into from
Dec 21, 2021
Merged

API/DOC/backwards: rework config defaults #125

merged 61 commits into from
Dec 21, 2021

Conversation

stsievert
Copy link
Owner

@stsievert stsievert commented Nov 16, 2021

What does this PR implement?
It changes the defaults for Salmon.

  • API: the values change, and there's also some easier methods to pass arguments to every sampler.
    • API: allow customizing the HTML title of query page
    • API: renames alg_ident to sampler.
  • DOC: the documentation around the defaults is much better.
  • DOC MAINT: including...
    • also adds to "why adaptive?"
    • cleans up docs around init file structure
    • clarifies samplers with same name.
    • expands upon examples (shows/tests default config, shows customization, etc)
  • TST: the config are tested too!
  • TST BUG: fixes CI testing! (required some backend work to make sure old endpoints weren't being hit after being deleted).
    • MAINT: properly sets number of processors
  • API: now YAML in /config can be uploaded to /init.
    • (technically some backwards incompatibility; n has been removed from /config).
  • TST: makes sure configs are proper experiments.
  • MAINT: only change docs on release (lesson learned)
  • MAINT/DOC: cleans up Runner/Sampler

Reference issues/PRs
Closes #124, closes #82 and closes #115.

@stsievert
Copy link
Owner Author

stsievert commented Nov 18, 2021

Some recommendations:

  • Make targets/ZIP file more clear (larger, earlier, etc).
  • Add clarification around the names in testing: ARR: {}.
    • mention this more explicitly in "Sampler configuration"?
  • Add link to "sampler configuration" (https://docs.stsievert.com/salmon/samplers.html) in API/Config and on "getting started" page.
  • Make more clear what "sampler" and "sampling" control.

@stsievert
Copy link
Owner Author

stsievert commented Nov 19, 2021

All tests pass locally for commit 72152a2. On CI, one test is failing (looks like a left-over from a previous test to make sure backend errors are logged).

@stsievert
Copy link
Owner Author

stsievert commented Nov 20, 2021

I've fixed the CI tests, and have accelerated them.

  • Fixing CI: launch max(4, NUM_PROCS) instead of NUM_PROCS Dask workers.
    • This is a problem because the GitHub Actions Docker machine only had 2 processes, meaning I'd get an index error when trying to access workers[2].
  • Acceleration: reduce sleeps. Biggest change: not sleeping while running rj.reset when hitting /reset.
    • This change went from 665s to 130s on my local machine

The time spent in the CI went from 37-38 minutes to 14-15 minutes.

clean up validation sampling test
better clearing of logs?

better clear logs

more printing

bg

same process
don't print

synchronous save

wrap save/bgsave to catch error

better sleep on save

don't sleep
narrow try/except
have some built in delay for Logs
bump

bump
@stsievert
Copy link
Owner Author

stsievert commented Nov 24, 2021

I've reworked resetting samplers. I think this closes #82. After hitting /reset, samplers would still live inside the FastAPI app. I reworked that by using a global variable on the condition that the backend server is run with one worker (and I added a note to the algorithm dev docs).

As a result, the CI works now.

@stsievert
Copy link
Owner Author

Now, hitting /reset?force=1 completely resets Salmon. I've modified the docs to reflect that, and have also reset the username/password (and have reopened #115).

@stsievert stsievert merged commit b4ec4b1 into master Dec 21, 2021
@stsievert stsievert deleted the config-defaults branch December 21, 2021 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant