zhengruifeng commented on PR #56584: URL: https://github.com/apache/spark/pull/56584#issuecomment-5018535835
One additional correctness concern with the current `skipTransient` approach: `@transient` does not mean that a field is shared or not owned by the model. `Word2VecModel` stores its trained `feature.Word2VecModel` in `@transient private val wordVectors`, does not override `estimatedSize`, and the generic ML cache path calls that inherited method. `fit` installs the learned vectors in this field; `transform` broadcasts it; and the model writer persists it. With `Model.estimatedSize` opting into `skipTransient`, a potentially large Word2Vec model is therefore reduced largely to metadata for cache accounting. The same implementation exists in runtime. This is a concrete counterexample to treating all transient fields as non-owned infrastructure. Please retain learned transient state in the estimate, e.g. by selectively excluding `Model.parent` rather than every transient field, and add a Word2Vec regression test whose estimate grows with vocabulary/vector size. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
