Update scheduling queries and unit tests to conform to Aerie v2.6 #129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
|
Looks like this is failing integration tests because we no longer have the 2.2.0 banananation jar. I can add this file back in, or we can update the tests to use the newest jar. I'm leaning towards the latter. |
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Error
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
cartermak
reviewed
Mar 26, 2024
There was a problem hiding this comment.
- I suggest you update the integration test defaults to also use 2.6.0 here. (I forgot to put that in the integration test documentation!)
- I think what's breaking the integration tests now is that, since Aerie 2.4.0ish, Aerie-CLI is no longer able to tell if the Aerie host has auth enabled. This is the method that used to work and needs to be updated.
src/aerie_cli/aerie_client.py
Outdated
Comment on lines
1525
to
1532
| # Note that as of Aerie v2.3.0, the metadata (incl. model_id and goal name) are stored in a separate table | ||
| formatted_upload_objects = [] | ||
| for entry in upload_object: | ||
| new_entry = {} | ||
| new_entry["definition"] = entry["definition"] | ||
| new_entry["metadata"] = {"data": {"name": entry["name"], "models_using": {"data": {"model_id": entry["model_id"]}}}} | ||
| formatted_upload_objects.append(new_entry) | ||
|
|
There was a problem hiding this comment.
For long-term maintainability, would it be better to break the format of upload_object than to hard-code this mapping? Who are the users of this method?
There was a problem hiding this comment.
The only user of this method is scheduling.upload. Is it preferable to deliver the upload_object to this method as it should be uploaded?
There was a problem hiding this comment.
I think that's preferable. I'd say we should avoid breaking changes to the API, but I don't know of anyone using the scheduling rule Python API.
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
had a problem deploying
to
integration-test-workflow
GitHub Actions
Failure
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
|
Update: I've modified this PR to also include updates to the constraint queries to bring them up to parity with the changes in v2.6.0. Also updated to fix the |
cartermak
reviewed
Apr 8, 2024
…sed and the same functionality is available with upload_scheduling_goals
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
joshhaug
temporarily deployed
to
integration-test-workflow
GitHub Actions
Inactive
cartermak
approved these changes
Apr 8, 2024
joshhaug
added a commit
that referenced
this pull request
Apr 11, 2024
* Update scheduling queries and unit tests to conform to Aerie v2.6 (#129) * Updating scheduling module to work with Aerie 2.6 * Update to include a 2.6 banananation jar, and updated .env to use 2.6 * Refactor to replace deprecated datetime.utcnow() calls * Removed banananation 2.2.0 jar * Updated to use 2.6.0 instead of 2.2.0 * Updated aerie_client based on PR feedback * Updating conftest to use v2.6.0 * Updated constraints queries to match v2.6.0 schema * Updated aerie_host.is_auth_enabled() * Updated constraints.py to remove unnecessary comments/add docstrings * Removed upload_scheduling_goal wrapper function, as it is not being used and the same functionality is available with upload_scheduling_goals * Documentation updates (#130) * Updated pyproject.toml version to v2.4.0 --------- Co-authored-by: Carter Mak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary motivation for this PR is to update the scheduling module to use the new db schemas introduced in Aerie v2.6.0. More information on these changes is available on the Aerie upgrade docs.
Additionally, there are also some cleanup tasks that I've lumped in:
.envfile accordinglydatetime.utcnow()calls with the new timezone-awaredatetime.now(timezone.utc). This is not a necessary change, but prevents us from throwing deprecation errors.I'm happy to split these up into separate PRs if preferred.