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

Add support for graph-transformer plugin #292

Merged
merged 12 commits into from
Feb 6, 2024
Merged

Add support for graph-transformer plugin #292

merged 12 commits into from
Feb 6, 2024

Conversation

CarlinLiao
Copy link
Collaborator

Will close #268.

Per the tests, something seems to have broken with the apiserver initialization that could use a second look over before merging.

@CarlinLiao CarlinLiao added the enhancement New feature or request label Feb 2, 2024
@CarlinLiao CarlinLiao self-assigned this Feb 2, 2024
@CarlinLiao
Copy link
Collaborator Author

Removing that stray import fixed the major apiserver issue, but the test verifying the change to the way importance scores are indexed is still failing.

@jimmymathews
Copy link
Collaborator

Can you report the logs from the tests that failed?

@CarlinLiao
Copy link
Collaborator Author

I don't have them on this computer, it'll have to wait until the evening or tomorrow. It was just the apiserver importance module test though. If I recall correctly the issue was that the data object parsed from the JSON returned from the API query was empty.

I force rebuilt my data loaded images so I don't think that was the issue.

@CarlinLiao
Copy link
Collaborator Author

Upon inspection, the current implementation of GraphsAccess could potentially return multiple feature specifications. We decided that we should break out getting one feature specification as a helper function to ensure that it only comes back with one feature specification.

@CarlinLiao
Copy link
Collaborator Author

I separated the feature specification fetch in GraphsAccess so it'd always only return one or zero specifications, but I can't figure out why the test fails now.

@CarlinLiao CarlinLiao marked this pull request as ready for review February 6, 2024 06:17
@CarlinLiao
Copy link
Collaborator Author

All tests pass. It took like 10 or 20 minutes, but the total time at the end showed 219393s or 2.5 days, which I can assure you is a slight overestimate. This isn't the first time either.

@jimmymathews
Copy link
Collaborator

Tests pass for me.

@jimmymathews jimmymathews merged commit 3e8de5f into main Feb 6, 2024
1 check passed
@CarlinLiao CarlinLiao deleted the issue268 branch February 6, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tmi2022 GNN model
2 participants