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

feat: Train: Adding code snippets #2591

Merged
merged 54 commits into from
Apr 5, 2023

Conversation

leiyre
Copy link
Member

@leiyre leiyre commented Mar 23, 2023

Description

This PR includes an access in the UI, from the header of the dataset, to view the training snippets in a modal window, and to allow copying them.

Closes #2475
Closes #2391

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested

  • new Base Tabs component test
  • update help Info component test

Checklist

  • I have merged the original branch into my forked branch
  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@davidberenstein1957
Copy link
Member

davidberenstein1957 commented Mar 25, 2023

Hi @leiyre,

Looks great already, I added some content snippets for everything I can think of currently.

Would it be possible to render the title as the name for the tab, within the UI? Currently, the tabs are pre-determined but IMO it could be better to show all files from 'docs/_source/_common/snippets/training' for each corresponding task.
Additionally, would it be possible to ignore files with a leading underscore? _ e.g. _spacy?
Also, would it be possible to pre-fill some variables like for example the dataset name, within the python code?

Additionally, I created this issue, perhaps it is already an option to add something simple for programmatic exporting in docs/_source/_common/snippets/exporting?

return libraries;
},
},
};
Copy link
Contributor

@keithCuniah keithCuniah Mar 29, 2023

Choose a reason for hiding this comment

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

performance: don't compute variables which don't need reactivity. This component have a lot of computeds. Maybe some or all can be put in static variables => see EditionUserInfo created hook. (if some value are not well reactive, you could use a conditional rendering at the level parent with a v-if)

let libraries = null;
try {
libraries = require.context(
`../../../../docs/_source/_common/snippets/training`,
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to use absolute and not relative import

Copy link
Member Author

Choose a reason for hiding this comment

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

it's outside of the project so I tried to create an alias with webpack for the path, but didn't work

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can define symlink to link and share the same folder in several places

@davidberenstein1957 @leiyre Is there any requirement to have these code snippets in the docs section? Will be shown there also?

If only code snippets are used in the frontend part, I would like to keep them as part of the UI static resources,
if not, we can even have a symbolic link inside the frontend folder and work as if they were there.

allow-close
@close-modal="showTrainModal(false)"
>
<DatasetTrainComponent :datasetTask="dataset.task" />
Copy link
Contributor

Choose a reason for hiding this comment

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

see the comment about replacing some or all computed props by static variable => could be interesting to add a v-if on DatasetTrainComponent

@davidberenstein1957 davidberenstein1957 changed the base branch from develop to releases/1.6.0 April 5, 2023 07:35
Copy link
Contributor

@tomaarsen tomaarsen left a comment

Choose a reason for hiding this comment

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

@davidberenstein1957 Could you try to verify if the parameters in the update_config are correct?

@davidberenstein1957 davidberenstein1957 merged commit ea51143 into releases/1.6.0 Apr 5, 2023
@davidberenstein1957 davidberenstein1957 deleted the feat/train_code_snippets branch April 5, 2023 12:26
@davidberenstein1957 davidberenstein1957 restored the feat/train_code_snippets branch April 5, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants