This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix the Scala URL issue for downloading data in scripts #12913
Merged
Merged
Changes from all commits
Commits
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 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 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 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
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.
I believe you are using S3 bucket to keep the model artifacts.
But, should the bucket name be mxnet-scala or something else ?
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.
@piyushghai Previously when I set up with the CI, I created a bucket namely
mxnet-scala
. There are alreadymxnet-model
ormxnet-models
in the S3 bucket. I don't want to add additional folder inside to change something there.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.
My point is, if tomorrow there was a similar example for another language binding, say Java, then they would either have to :
OR
I don't think it's a scalable to follow point 1, which we seem to be doing here, because then we will end up having to duplicate hosting of files at other places as well.
There are also other cases, where let's say one wants to replace a trained model with a new state of the art trained model, the fewer the number of places in which this change happens, the easier it will be right ?
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.
But the fact is that some of the models with the same name cannot be used in Scala/Java. Such as Res152. Not all res152 models from MXNet Python can be successfully ported into MXNet Scala/Java especially those in data.mxnet.io. The cause behind it can be some diff on the precision (Float64) or input-type diffs. In that case, there has to be a Scala/Java model zoo somewhere to holds the code. Since Java is the child came from Scala, it should be fine for it to use the model directly from Scala.
I think model-server they have their own model zoo is also with the same reason, or they can port all models from one place directly.
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 see your point there. But we might not be using the best practice here.
I'll [reluctantly] approve this PR since the break is a critical break and fixes a bunch of malformed urls in examples
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.
its ok for now, I agree with @piyushghai that we need to make sure the same models work across all language bindings, otherwise it is confusing to users why the models are different and could lose their trust.