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.

Sign up for GitHub

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

Jump to bottom

add missing mandatory parameters for generate_data #50

Merged
merged 1 commit into from
Oct 7, 2024
Merged

add missing mandatory parameters for generate_data #50

merged 1 commit into from
Oct 7, 2024

Conversation

JudeNiroshan
Copy link
Contributor

When running SDG with against taxonomy-base "empty", SDG failed due to missing mandatory parameters.

Users can trigger full SDG workflow by providing an arbitrary negative number as the pr number when initiating a pipeline run.

leseb
leseb reviewed Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

I confirm that I had the same problem when either repo_pr or repo_branch were not set and setting both chunk_word_count and server_ctx_size fixed it.

LGTM - minus the comment about pipeline.yaml changing.

pipeline.yaml Outdated
Comment on lines 616 to 618
] && [ {{$.inputs.parameters[''repo_pr'']}} -gt 0 ]; then git fetch origin
pull/{{$.inputs.parameters[''repo_pr'']}}/head:{{$.inputs.parameters[''repo_pr'']}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why these lines changed, any idea? git_clone_op was not modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's my mistake. In this PR, I have forgot to generate the pipeline.yaml after this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks for the clarification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JudeNiroshan can you remove this change from this PR? Then we can merge it 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should have #55 first, to fix the pipeline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok #55 is merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, @JudeNiroshan please rebase.

@leseb
Copy link
Collaborator

leseb commented Oct 3, 2024

Please install https://docs.astral.sh/ruff/installation/ on your system and run ruff format . once #57 is merged. THEN rebase your PR, everything should go fine :).

@cooktheryan cooktheryan merged commit 73592d9 into redhat-et:main Oct 7, 2024
sallyom pushed a commit to sallyom/ilab-on-ocp that referenced this pull request Oct 8, 2024
@cooktheryan @sallyom
Merge pull request redhat-et#50 from JudeNiroshan/add_mandatory_params
ea91195 
add missing mandatory parameters for generate_data
sallyom pushed a commit to sallyom/ilab-on-ocp that referenced this pull request Oct 8, 2024
@cooktheryan @sallyom
Merge pull request redhat-et#50 from JudeNiroshan/add_mandatory_params
b473e9d 
add missing mandatory parameters for generate_data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb approved these changes

@MichaelClifford MichaelClifford Awaiting requested review from MichaelClifford

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JudeNiroshan @leseb @MichaelClifford @cooktheryan