To go along with the breaking changes proposed in https://github.com/aws/sagemaker-python-sdk/issues/1459, we should provide a script that makes it easier to update one's code. For example, TensorFlow did this for upgrading to TF 2.0 (source).
Hi @laurenyu , would love to contribute to this feature. Are external contributors allowed in the migration? If yes, how can I help?
@Olamyy yes! I've just posted a PR (#1492) to start laying some ground work. Take a look, comment with suggestions for refactoring, renaming, etc.
after that, I think the next steps would include:
ASTTransformer class, and dumping the results into a new fileanything of the above jump out as something you'd like to pick up?
I went through the ASTTransformer PR and it seems fine to me.
I want to try and move on to setting up tests.
Should these tests be ASTTransformer related or general v2 migration tests?
@Olamyy just for the ASTTransformer and code related to this migration script
edit: since I've merged some more code, just wanted to clarify - we're looking for some unit tests for testing the stuff in tools/compatibility/v2 that can also be run from buildspec-unittests.yml. You don't have to fill in tests for everything that's been merged this week, though - I can definitely help out with that :)
@Olamyy have you had a chance to try working on things here?
Hi @laurenyu , I currently have a setup here https://github.com/Olamyy/sagemaker-python-sdk/tree/v2-compability-tests/tests/unit/v2 where I plan to write the tests. I've been able to write a simple function to test the output of the ASTTransformer. here. However, i'm honestly not sure what other thing to test. Could you provide some tips on how you'll go about this for me? This would help me decide what tests I need to write and which are not important.
@Olamyy what you've got is good; I didn't have too much in mind at the time because, you're right, there's not a lot of code in ASTTransformer. I think I'd also add a test case where there shouldn't be any change with the code.
The other thing is that we use pytest rather than unittest. I added some tests last week for my more recent changes, so that can help with seeing how some of the unit tests in this repo are written: https://github.com/aws/sagemaker-python-sdk/tree/zwei/tests/unit/sagemaker/cli/compatibility/v2
Hi @laurenyu , I made a PR with the tests here https://github.com/aws/sagemaker-python-sdk/pull/1555 .
Please, let me know if there's any fix I need to add.
as we're nearing the release of v2.0, noting what's left:
Most helpful comment
Hi @laurenyu , I made a PR with the tests here https://github.com/aws/sagemaker-python-sdk/pull/1555 .
Please, let me know if there's any fix I need to add.