-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: make silent job and peer failures visible and actionable #9497
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
Open
pjwerneck
wants to merge
14
commits into
dev
Choose a base branch
from
pjwerneck/fix-silent-failures
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,455
−134
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
0ebb07e
fix: make silent job and peer failures visible and actionable
pjwerneck 288ec39
Merge remote-tracking branch 'origin/dev' into pjwerneck/fix-silent-f…
koenvanderveen 6b53ac6
Merge remote-tracking branch 'origin/dev' into pjwerneck/fix-silent-f…
koenvanderveen e6398dd
Merge pull request #9501 from OpenMined/koen/pr-review-skill-standards
koenvanderveen b964e32
Merge pull request #9502 from OpenMined/koen/pr-review-skill-standards
koenvanderveen bd1ebb0
Merge pull request #9504 from OpenMined/koen/pr-review-skill-naming
koenvanderveen f316fac
Merge pull request #9505 from OpenMined/koen/pr-review-skill-naming
koenvanderveen 8fd723f
fix: update job indexing to use datasite and job name
pjwerneck 7a4af9b
fix: update job referencing to use email instead of datasite for clarity
pjwerneck d29a2fd
fix: revert wrong pre-check, move tests
pjwerneck 8d310be
test: split job addressing tests for hinting and indexing
pjwerneck 4457bb1
fix: prettier pre-commit fail
pjwerneck f65467e
chore: number every review section and give each PR its own review fo…
koenvanderveen 3221c65
Merge branch 'dev' into pjwerneck/fix-silent-failures
pjwerneck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| """Unit tests for EnclaveJobInfo, the per-party approval gate. | ||
|
|
||
| The gate lives here rather than in SyftEnclaveClient.approve_job, so these | ||
| build a job on a tmp_path SyftBox folder instead of a four-party enclave flow. | ||
| """ | ||
|
|
||
| from datetime import datetime, timezone | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from syft_enclaves.enclave_job_info import ( | ||
| EnclaveJobInfo, | ||
| PartyApprovalStatus, | ||
| enclave_approval_file_name, | ||
| ) | ||
| from syft_job.client import JobClient | ||
| from syft_job.config import SyftJobConfig | ||
| from syft_job.job import JobInfo | ||
| from syft_job.job_storage import JobRef | ||
| from syft_job.models import JobState, JobStatus, JobSubmissionMetadata | ||
|
|
||
| DO_EMAIL = "do@test.org" | ||
| DS_EMAIL = "ds@test.org" | ||
|
|
||
|
|
||
| def _make_enclave_job(tmp_path: Path, job_name: str = "test_job") -> EnclaveJobInfo: | ||
| """An enclave job on the DO's datasite, with no approval file written yet.""" | ||
| syftbox = tmp_path / "SyftBox" | ||
| syftbox.mkdir() | ||
| client = JobClient( | ||
| config=SyftJobConfig(syftbox_folder=syftbox, current_user_email=DO_EMAIL) | ||
| ) | ||
| ref = JobRef( | ||
| datasite_email=DO_EMAIL, | ||
| ds_email=DS_EMAIL, | ||
| job_name=job_name, | ||
| protocol_version="1", | ||
| ) | ||
| job = JobInfo( | ||
| job_metadata=JobSubmissionMetadata( | ||
| name=job_name, | ||
| type="python", | ||
| submitted_by=DS_EMAIL, | ||
| datasite_email=DO_EMAIL, | ||
| submitted_at=datetime.now(timezone.utc), | ||
| ), | ||
| state=JobState(status=JobStatus.PENDING), | ||
| client=client, | ||
| current_user_email=DO_EMAIL, | ||
| ref=ref, | ||
| ) | ||
| return EnclaveJobInfo.from_job_info(job) | ||
|
|
||
|
|
||
| def test_approve_refuses_when_approval_file_missing(tmp_path: Path): | ||
| """No approval file means the enclave has not distributed the job yet. | ||
|
|
||
| The message used to say the caller may not be a designated party, which is | ||
| the wrong cause for the common case and offers nothing to do about it. | ||
| """ | ||
| job = _make_enclave_job(tmp_path) | ||
|
|
||
| with pytest.raises(PermissionError) as exc: | ||
| job.approve() | ||
|
|
||
| message = str(exc.value) | ||
| assert DO_EMAIL in message | ||
| assert "test_job" in message | ||
| assert "client.sync()" in message | ||
|
|
||
|
|
||
| def test_approve_refuses_when_already_approved(tmp_path: Path): | ||
| """A second approval must not overwrite the first one's timestamp.""" | ||
| job = _make_enclave_job(tmp_path) | ||
| approval_file = job.job_review_path / enclave_approval_file_name(DO_EMAIL) | ||
| PartyApprovalStatus(party=DO_EMAIL).save_json(approval_file) | ||
|
|
||
| job.approve() | ||
| assert PartyApprovalStatus.load_json(approval_file).status == JobStatus.APPROVED | ||
|
|
||
| with pytest.raises(ValueError, match="Already in status: approved"): | ||
| job.approve() |
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JobClient.jobsis now the single authority for order — own datasite first, then owner email, then newest — and both the printed[N]andjobs[N]read that one list, so they agree.I think in general we need 1) a deterministic way to get job X from user Y, ideally
client.jobs["a@b.org"]["<jobname>"], and 2) to use that in the hints.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because fixing the hints kind of solve a problem that shouldnt exist in the first place: no deterministic way to get a job handle, we may sometimes use the shorthand but we shouldnt all the time. If we can assert that there is no @ in a job name, we would know whether a passed arg is an email or a job name, filter on emails first and then filter on job name, which should be unique for that user
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
jobs["do@org.com"]["analysis"]. An @ in the key means datasite, so validate_job_name rejects @ in new job names now, as you suggested, but it handles existing ones. I added a deprecation warning.Chaining a second email narrows to the ds when a name is duplicated among submitters, so
jobs["do@x"]["ds1@y"]["analysis"]disambiguates.