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

Allow target function to output columns already present in the data set #3214

Merged
merged 5 commits into from May 13, 2024

Conversation

nick863
Copy link
Contributor

@nick863 nick863 commented May 11, 2024

Description

Currently target function will fail if its output column will be present in the input data set. In this PR we are fixing it. See work item #3170343.

All Promptflow Contribution checklist:

  • The pull request does not introduce [breaking changes].
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request to get dedicated review from promptflow team. Learn more: suggested workflow.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@nick863 nick863 requested a review from a team as a code owner May 11, 2024 05:18
Copy link

github-actions bot commented May 11, 2024

promptflow-evals test result

  9 files    9 suites   2h 11m 6s ⏱️
 32 tests  24 ✅  8 💤 0 ❌
288 runs  216 ✅ 72 💤 0 ❌

Results for commit 9badde3.

♻️ This comment has been updated with latest results.

@nick863 nick863 merged commit 6ab1286 into main May 13, 2024
35 checks passed
@nick863 nick863 deleted the nirovins/allow_output_as_input branch May 13, 2024 23:10
Comment on lines +318 to +329
for evaluator_name, mapping in evaluator_config.items():
mapped_to_values = set(mapping.values())
for col in target_generated_columns:
# If user defined mapping differently, do not change it.
# If it was mapped to target, we have already changed it
# in _process_evaluator_config
run_output = f'${{run.outputs.{col}}}'
# We will add our mapping only if
# customer did not mapped target output.
if col not in mapping and run_output not in mapped_to_values:
evaluator_config[evaluator_name][col] = run_output

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we would need the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case we are making sure, we map all the outputs of target function to the apropriate columns in the evaluators input, however, we should not do that if customer have mapped this output to something else.

@nick863 nick863 mentioned this pull request May 14, 2024
7 tasks
nick863 added a commit that referenced this pull request May 14, 2024
# Description

This PR contains follow up changes for PR
#3214

# All Promptflow Contribution checklist:
- [x] **The pull request does not introduce [breaking changes].**
- [x] **CHANGELOG is updated for new features, bug fixes or other
significant changes.**
- [x] **I have read the [contribution guidelines](../CONTRIBUTING.md).**
- [x] **Create an issue and link to the pull request to get dedicated
review from promptflow team. Learn more: [suggested
workflow](../CONTRIBUTING.md#suggested-workflow).**

## General Guidelines and Best Practices
- [x] Title of the pull request is clear and informative.
- [x] There are a small number of commits, each of which have an
informative message. This means that previously merged commits do not
appear in the history of the PR. For more information on cleaning up the
commits in your PR, [see this
page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).

### Testing Guidelines
- [x] Pull request includes test coverage for the included changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants