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

Moving config from ADS to UI for unverified models #976

Merged
merged 20 commits into from
Oct 29, 2024

Conversation

kumar-shivam-ranjan
Copy link
Member

@kumar-shivam-ranjan kumar-shivam-ranjan commented Oct 22, 2024

This PR is intended to remove dependency of unverified models default config from ADS.
Currently, the default shapes config for deployment, finetuning and evaluation are hardcoded in ADS. This makes it difficult to make changes ad-hoc with new ADS release.
This info is needed for unverified models only and can be maintained at client side.
Config that needs to be removed are

UTs

Screenshot 2024-10-29 at 12 45 21 AM

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 22, 2024
Copy link
Member

@VipulMascarenhas VipulMascarenhas left a comment

Choose a reason for hiding this comment

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

are we rolling out the relevant UI changes this week? If that's the case, we can set the target branch to main and release ADS this Thursday. That way, the next NBVM update in a week would have these changes.

logger.info(f"Fetching default fine-tuning config for model: {model_id}")
config = get_finetuning_config_defaults()
logger.info(
f"default fine-tuning config will be used for model: {model_id}"
Copy link
Member

Choose a reason for hiding this comment

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

this info message can be misleading since user might see this in ads logs and expect some default to be returned, but the defaults are applied outside of ads. Maybe a better phrasing would be "Fine-tuning config for custom model {model_id} is not available." We can also use logger.debug instead, user need not see this message every time.
@mrDzurb thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

AQUA_MODEL_DEPLOYMENT_CONFIG,
AQUA_MODEL_DEPLOYMENT_CONFIG_DEFAULTS,
Copy link
Member

Choose a reason for hiding this comment

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

can you please the unused constants and json files/functions that were deprecated here since we're not using them anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry didn't understand this statement. perhaps you a word or two?

Copy link
Member

@VipulMascarenhas VipulMascarenhas Oct 22, 2024

Choose a reason for hiding this comment

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

I missed the word - " can you please remove or delete the unused constants and json files/functions that were deprecated..." :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure , will update

@kumar-shivam-ranjan
Copy link
Member Author

are we rolling out the relevant UI changes this week? If that's the case, we can set the target branch to main and release ADS this Thursday. That way, the next NBVM update in a week would have these changes.

UI changes are ready. I was under assumption that only embedding changes + adding logs in terminal are part of release this week. But if we plan to release config migration changes as well , we can.

Looping @mayoor @NitinMore7

@mrDzurb
Copy link
Member

mrDzurb commented Oct 23, 2024

Please add more details in the description. It will be helpful for the future maintenance.

@VipulMascarenhas
Copy link
Member

are we rolling out the relevant UI changes this week? If that's the case, we can set the target branch to main and release ADS this Thursday. That way, the next NBVM update in a week would have these changes.

UI changes are ready. I was under assumption that only embedding changes + adding logs in terminal are part of release this week. But if we plan to release config migration changes as well , we can.

Looping @mayoor @NitinMore7

yes, let's switch target to main if we're comfortable with the UI changes to go this week.
cc : @mayoor

mrDzurb
mrDzurb previously approved these changes Oct 25, 2024
@kumar-shivam-ranjan kumar-shivam-ranjan changed the base branch from feature/aqua-v1.0.5 to main October 28, 2024 16:16
@kumar-shivam-ranjan kumar-shivam-ranjan dismissed mrDzurb’s stale review October 28, 2024 16:16

The base branch was changed.

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-20.15%

mrDzurb
mrDzurb previously approved these changes Oct 28, 2024
Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-20.15%

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-20.15%

Copy link

📌 Cov diff with main:

Coverage-45%

📌 Overall coverage:

Coverage-58.95%

@kumar-shivam-ranjan kumar-shivam-ranjan self-assigned this Oct 28, 2024
@kumar-shivam-ranjan kumar-shivam-ranjan merged commit 5b52216 into main Oct 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants