Skip to content

Conversation

@mikelawrence-google
Copy link

@mikelawrence-google mikelawrence-google commented Sep 12, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes b/239609625 🦕

@mikelawrence-google mikelawrence-google requested a review from a team as a code owner September 12, 2022 23:53
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: vertex-ai Issues related to the googleapis/python-aiplatform API. labels Sep 12, 2022
@mikelawrence-google mikelawrence-google changed the title Adds the temporal fusion transformer (TFT) forecasting job feat: Adds the temporal fusion transformer (TFT) forecasting job Sep 12, 2022
@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@mikelawrence-google mikelawrence-google force-pushed the mikealawrence-add-tft-model-support branch from e7cdd79 to b19d2c4 Compare September 12, 2022 23:55
@mikelawrence-google
Copy link
Author

@sasha-gitg Hi there, I need a little help and I see you've done a lot of work in this repo. Do you often have this presubmit issue with pytest like,

_____________________________ ERROR collecting gw2 _____________________________ Different tests were collected between gw3 and gw2. The difference is: 

I don't think my PR has done anything particularly wild... I'm not sure how to fix this. Any ideas? If you think someone else might be better to ask, feel free to let me know

@sasha-gitg
Copy link
Member

@mikelawrence-google Looks like an issue with python-xdist and @pytest.mark.parametrize with a set: pytest-dev/pytest-xdist#149

Try changing back to list or following the sorted recommendation in the issue.

@sararob sararob added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 15, 2022
@sararob sararob removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 15, 2022
@mikelawrence-google
Copy link
Author

Thanks for taking a look, @sasha-gitg and @sararob. I thought it was more related to build infrastructure, but it was indeed the set vs list issue. My team is waiting on some documentation before we can merge, but will get the review going

Copy link
Contributor

@geraint0923 geraint0923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the SDK support for TFT!

training_jobs.SequenceToSequencePlusForecastingTrainingJob,
marks=pytest.mark.skip(reason="Seq2Seq not yet released."),
training_jobs.TemporalFusionTransformerForecastingTrainingJob,
marks=pytest.mark.skip(reason="TFT not yet released."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will TFT be released? We should hold merging until it is released.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend is ready, we are just waiting for documentation and some UI pieces. It's completely usable though. My TL suggested we merge it ahead of time with the understanding that customers can use it, just without the GA guarantees at launch time (~1 month out). What do you think?

@nayaknishant nayaknishant added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 30, 2022
@sararob sararob removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 4, 2022
@sararob
Copy link
Contributor

sararob commented Oct 8, 2022

We are doing maintenance on our repo starting today and need to close all open PRs. When this is done next week, you'll be able to open a new PR.

@sararob sararob closed this Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: vertex-ai Issues related to the googleapis/python-aiplatform API. size: m Pull request size is medium.

7 participants