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

[DRAFT] Restructuring the code #383 #415

Merged
merged 26 commits into from
Jun 9, 2023
Merged

[DRAFT] Restructuring the code #383 #415

merged 26 commits into from
Jun 9, 2023

Conversation

Jeadie
Copy link
Contributor

@Jeadie Jeadie commented May 17, 2023

Changing code structure for #383

Still in draft, fielding consenus/feedback.

Further refactors will be, but not included in this:

  • Refactor config.yml to have a default tag for common params.
  • Use FROM ann-benchmarks-abc for algorithm XYZ when applicable

@erikbern
Copy link
Owner

This is awesome! Let me know how we can get this merged. Think this structure is a lot cleaner.

@Jeadie
Copy link
Contributor Author

Jeadie commented May 23, 2023

No problem. I will try getting this ready for merge this week (to avoid ongoing merge conflicts). Currently working on this. Right now what is left to do it just minimally reimplement definition.py. I think there's better future refactors for this, but I want this PR to provide the least amount of logic changes.

@Jeadie
Copy link
Contributor Author

Jeadie commented May 26, 2023

Okay, changes are up. I have had a few problems with some of the tests in benchmarks.yml. All except for sptag are transient, that is they have at least one (generally more) passing attempts in my action history (once completing the featureset, of course). SPTAG seems to be the one consistently to fail.

@@ -5,6 +5,7 @@ on:
branches:
- main
pull_request:
workflow_dispatch:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe workflow_dispatch is only allowed for write accessed users (i.e. safe for open-source repo) [link].

@Jeadie
Copy link
Contributor Author

Jeadie commented Jun 7, 2023

@erikbern fixes in for restructure. Hopefully we can get it merged before more conflicts.

GH action run. SPTAG failing for build problems.

@erikbern
Copy link
Owner

erikbern commented Jun 8, 2023

This looks AMAZING!! I Would love to merge this asap

Sorry for being slow to check in on this – I've been super busy. Let me know if I can help with this in any way?

@Jeadie
Copy link
Contributor Author

Jeadie commented Jun 8, 2023

No problem, I understand early startup days can be intense. I often get transient or odd errors in various algorithm specific tests. Will just want to make sure either a) you know the issue with a specific test, or b) you accept they won't be working in this PR run and we can work on improving/fixing post-merge.

@erikbern erikbern merged commit 3cae098 into erikbern:main Jun 9, 2023
@erikbern
Copy link
Owner

erikbern commented Jun 9, 2023

Let's merge this – I think it puts it in a better place for further work! Thanks a lot for this contribution!!!

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.

2 participants