-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(backend): stop compressing artifacts #1640
feat(backend): stop compressing artifacts #1640
Conversation
DIsabled compressing artifacts to .tar.gz archives. Fixes kubeflow#1383
@rileyjbauer Can you please check that UI and visualizations work fine when the artifacts are not compressed? |
@rileyjbauer I find it surprising that e2e frontend tests do not fail change. Can you please take a look? |
/retest |
I've changed the PR to only stop compressing the non-metadata artifacts. So this should be safe. |
/retest |
After you stopped compressing those artifacts, is there any code that could break? I'm not very clear on where these artifacts are used. |
The UX that shows the artifacts can break with high probability. @IronPan WDYT? How does the backend API handler compressed artifact blobs? What about directories? |
Any update on this PR? It would be very useful! |
I think one problem right now is that some APIs like ReadArtifact probably only handle single files and will fail with directories. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This is sth I'd want to try sooner or later |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ark-kun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
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.
I think this is a better onboarding experience, also UX should already support this thanks to @eterna2's previous improvements.
I'd still want to make sure archive is an option, because text files can have a large compress rate, so it saves network IO. Can we add this as an option to API server config?
@@ -218,6 +218,8 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) { | |||
for artIdx, artifact := range template.Outputs.Artifacts { | |||
if artifact.Name == "mlpipeline-ui-metadata" || artifact.Name == "mlpipeline-metrics" { | |||
workflow.Workflow.Spec.Templates[templateIdx].Outputs.Artifacts[artIdx].Optional = true | |||
} else { | |||
artifact.Archive = &workflowapi.ArchiveStrategy{None: &workflowapi.NoneStrategy{}} |
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.
nit: why do we add this in an else clause here?
I think we can add it to all.
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.
Another issue is that, there should be some similar code in CreateJob
method, you should add this part there too
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.
nit: why do we add this in an else clause here?
I'm not sure that Frontend and Backend Metrics are prepared for uncompressed artifacts.
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.
Frontend server should support both compressed and uncompressed artifacts.
pipelines/frontend/server/minio-helper.ts
Line 134 in 4e5d724
export async function getObjectStream({ |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
7542f0e
to
44d22a6
Compare
This PR should also resolve #5558 |
@eterna2 in v2 compatible mode, artifacts are already not compressed by default |
Nice! I was reading thru the codes for v2. Am very excited with all the new stuff coming up! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Checks whether env var awsAnonymousCredential true to use S3 resource config anonymous addresses issue kubeflow#1581 Signed-off-by: Malini Bhandaru <mbhandaru@vmware.com>
Closing this PR. No activity for more than a year. /close |
@rimolive: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
DIsabled compressing artifacts to .tar.gz archives.
Fixes #1383
This change is