Skip to content
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

Changed type of _fillna_map in normalize_time_series #4197

Closed
wants to merge 1 commit into from

Conversation

duartepinto555
Copy link

Issue #, if available:

Description of changes:
Changed the _fillna_map format to Timestamp with UTC. Previously this was changing the type of the series if the _fillna_map was not in the same format as the series, which later causes an error in the ".dt" parameter.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…series

Issue #, if available:

Description of changes:
Changed the _fillna_map format to Timestamp with UTC. Previously this was changing the type of the series if the _fillna_map was not in the same format as the series, which later causes an error in the ".dt" parameter.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
@yinweisu
Copy link
Collaborator

Previous CI Run Current CI Run

@duartepinto555
Copy link
Author

I'm sorry, this is my first time contributing on github, I was just running code with autogluon and arrived upon this error and thought of giving a contribute... All the tests still pass and around the same time, should I do something else? Sorry for the inconvenience

Copy link

Job PR-4197-1b62216 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-4197/1b62216/index.html

@Innixma
Copy link
Contributor

Innixma commented May 16, 2024

Hey @duartepinto555, great find, thanks for sending a PR!

Could you provide a code example where the previous logic fails but your PR succeeds?

You can either include it in the PR as a unit test or provide the code in a GitHub comment in this PR and I can test it out.

The main thing I need is to be able to verify that this change fixes a scenario where we previously failed.

@Innixma Innixma added this to the 1.1.1 Release milestone May 16, 2024
@duartepinto555
Copy link
Author

duartepinto555 commented May 17, 2024

Thank you for your detailed answer, @Innixma!

So, my use case was a niche one.

Basically, I have a model generated from AWS from an older autogluon==0.4.3. This made the _fillna_map attribute of the DatetimeFeatureGenerator differ from the new version. In version 0.4.3, the attribute doesn't have the datetime with "utc" which changes the type when comparing to the new version.

The error appears when loading this new model using autogluon==1.1.0 and trying to get a prediction. I can share my code as follows:

from autogluon.tabular import TabularPredictor
import pandas as pd
import os

MODEL_DIR = 'path_to_model_from_version_0.4.3'
testing_dataset_dir = 'path_to_prediction_dataset.feather'

# Loading model
predictor = TabularPredictor.load(MODEL_DIR, require_version_match=False)
predictor._decision_threshold = None

# Loading training dataset
df = pd.read_feather(testing_dataset_dir)

# Change paths from AWS S3 to Local one
model_name = 'CatBoost_BAG_L1_FULL'
predictor._trainer.set_model_attribute(model_name, 'path', [model_name])

# Get prediction
prediction = predictor.predict(df, model=model_name)    # >> Error occurs here

Hope you can still replicate this error, if you want me to provide an older model which returns this error I can do so as well!

@Innixma
Copy link
Contributor

Innixma commented May 17, 2024

Ah I see, thanks for the response @duartepinto555. The proposed change would introduce a inference latency overhead due to the extra pd.to_datetime conversion. While I would merge it if it were resolving an existing bug, this is instead related to backwards compatibility.

We explicitly do not support backwards compatible model loading, which is why when you try to load the old artifact it will warn you that the versions differ. Trying to ensure our code works when loading old artifacts is too complicated and would slow down our development, especially for versions that differ in major version (0.x <-> 1.x). My guess is that the bug you are experiencing here is just the tip of the iceberg, and many other things would go wrong trying to get your old model working on 1.1.0 without retraining.

Please either retrain your predictor from scratch using 1.1.0, or continue using 0.4.3 for inference. You can try monkey-patching / using your own fork of AutoGluon with this change, but you'll have to find your own work-arounds to any issues going this route.

If your model artifact is from SageMaker AutoPilot / Canvas, I believe they will be upgrading to using a more recent version of AutoGluon shortly (within 1-2 months).

@Innixma Innixma closed this May 17, 2024
@Innixma Innixma added the wontfix This will not be worked on label May 17, 2024
@duartepinto555 duartepinto555 deleted the patch-1 branch May 18, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants